mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 60252: Fixed a bug that causes segfault in ProcessManager::finalize.
Date Thu, 22 Jun 2017 18:29:13 GMT


> On June 21, 2017, 11:42 a.m., Joseph Wu wrote:
> > I agree this makes the finalization logic safer, but I don't see a way to add a
`nullptr` to the `ProcessManager::processes` map.
> > 
> > `ProcessManager::spawn` is the only location where the `processes` map is inserted,
and this method CHECK-fails if you give it a `nullptr`.  Two other locations use the map's
`operator[]`, but those are guarded by a mutex so they will never empty-initialize a map entry.
> > 
> > Can you note how you ran into a segfault here?  It's more likely that the segfault
was caused by a dereferencing a non-`nullptr` process that had already been deallocated (which
would not be fixed by this review).
> 
> Jiang Yan Xu wrote:
>     The use case is something like this: 
>     https://github.com/apache/mesos/blob/65152413836b58d01ace3a40bdc9056f9a489c6b/3rdparty/libprocess/src/tests/process_tests.cpp#L691
>     
>     Arguably I made an error in doing:
>     
>     ```
>     ProcessBase process;
>     UPID pid = spawn(&process, true);
>     
>     ...
>     
>     terminate(process);
>     wait(process);
>     ```
>     
>     And the process (in a test method) goes out of scope before libprocess finalize()
so it segfaulted.
>     
>     Just felt it's not really necessary to fail (and it used to not) in this way?
> 
> Joseph Wu wrote:
>     I see.  That error will still segfault, even with this patch.
>     
>     ---
>     
>     The tests (and otherwise invalid/risky uses of libprocess methods) did not segfault
in the past because we didn't clean up libprocess in the past.

Hmm you are right it's true that this wouldn't help. Discarding this. It'll probably not be
much safer without using `shared_ptr`s.


- Jiang Yan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/60252/#review178522
-----------------------------------------------------------


On June 20, 2017, 4:16 p.m., Jiang Yan Xu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60252/
> -----------------------------------------------------------
> 
> (Updated June 20, 2017, 4:16 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We don't need and shouldn't assume pointers in `processes` are
> non-nullptr.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/process.cpp 7ce6d2b13baa68906e091a95c0dd58ee1ca2bc7d 
> 
> 
> Diff: https://reviews.apache.org/r/60252/diff/1/
> 
> 
> Testing
> -------
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message