> On Oct. 25, 2018, 8:07 a.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.
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?
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69123/#review210024
-----------------------------------------------------------
On Oct. 26, 2018, 3: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, 3: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
>
>
