mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 69123: Fixed an early fd close in the cgroups event notifier.
Date Mon, 29 Oct 2018 21:18:20 GMT


> On Oct. 25, 2018, 4:07 p.m., Qian Zhang wrote:
> > src/linux/cgroups.cpp
> > Lines 1071-1080 (original), 1071-1089 (patched)
> > <https://reviews.apache.org/r/69123/diff/1/?file=2101988#file2101988line1071>
> >
> >     I see we already have an onAny callback `_listen`, can we close the fd at the
end of that method (i.e., when we are sure that we will not listen anymore)? And then we could
change the code here like below:
> >     ```
> >     if (reading.isSome()) {
> >       reading->discard();
> >     } else if (eventfd.isSome()) {
> >       Try<Nothing> unregister = unregisterNotifier(eventfd.get());
> >       if (unregister.isError()) {
> >         LOG(ERROR) << "Failed to unregister eventfd: " << unregister.error();
> >       }
> >     }
> >     
> >     ```
> 
> Benjamin Mahler wrote:
>     I originally tried to simplify the lifecycle of the listener, but it was implemented
this way because some callers call listen multiple times.
>     
>     create listener
>     listen
>     listen
>     listen
>     terminate
> 
> Qian Zhang wrote:
>     Yeah, I know we support that. So that's why I suggested to close the fd (`unregisterNotifier(eventfd.get())`)
at the end of `_listen`, and then even the caller call `listen` again, we will actually do
nothing but just return `error` (since `error` has been set in `_listen`). I think if we are
sure that we will not listen again (i.e., the first time when `_listen` is call with `error`
set), we should close the fd immediately rather than closing it until `finalize` is called.
> 
> Benjamin Mahler wrote:
>     The code that was suggested will leak the fd:
>     
>     ```
>     if (reading.isSome()) {
>       reading->discard();
>     } else if (eventfd.isSome()) {
>       Try<Nothing> unregister = unregisterNotifier(eventfd.get());
>       if (unregister.isError()) {
>         LOG(ERROR) << "Failed to unregister eventfd: " << unregister.error();
>       }
>     }
>     ```
>     
>     If we're in `finalize()` and `reading.isSome()`, this would never close the fd since
the dispatch to `_listen` will be dropped (the Process terminated).
>     
>     Are you suggesting something other than this code?

You are right, that will leak the fd, so I think we should go with your solution. But I'd
still suggest to close the fd at the end of `_listen` since we are sure the listener will
not listen again (i.e., the fd will not be used by the listener anymore). How do you think?


- Qian


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


On Oct. 26, 2018, 11:25 a.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69123/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2018, 11:25 a.m.)
> 
> 
> Review request for mesos and Qian Zhang.
> 
> 
> Bugs: MESOS-9334
>     https://issues.apache.org/jira/browse/MESOS-9334
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The cgroups event notifier was closing the eventfd while an
> `io::read()` operation may be in progress. This can lead to
> bugs where the fd gets re-used and read from a stale io::read.
> 
> 
> Diffs
> -----
> 
>   src/linux/cgroups.cpp 95efc1def3318dd93d30d91e9c8a91376cc610b8 
> 
> 
> Diff: https://reviews.apache.org/r/69123/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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