mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 67137: Avoided leaking file descriptors in Mesos containerizer.
Date Tue, 15 May 2018 20:23:36 GMT


> On May 15, 2018, 8:28 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 481 (patched)
> > <https://reviews.apache.org/r/67137/diff/1/?file=2023058#file2023058line481>
> >
> >     This kind of logic will _never_ work on Windows, because the file descriptors
are actually handles, and you can't just iterate through all open handles.

It isn't clear to me whether we can provide this functionality (close all open, non-whitelisted
file descriptors) with the same implementation. If we cannot just iterate all file descriptors,
we would need to fall back to platform-specific solutions to list the actually open file descriptors
(e.g., `/proc/self/fd` on Linux, `/dev/fd` on macos, another API on Windows).

Before merging this it probably makes sense to benchmark an approach explicitly passing in
a list of file descriptors to close instead of iterating. It might both have a better runtime
behavior, and also allows us more clearly separate out platform-dependent code.


> On May 15, 2018, 8:28 p.m., Andrew Schwartzmeyer wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 1110-1113 (patched)
> > <https://reviews.apache.org/r/67137/diff/1/?file=2023058#file2023058line1110>
> >
> >     Why not just `O_CLOEXEC` all the handles opened on Linux (so that you can avoid
all this kind of hacky logic here)? (And fix the third-party libs to do the same.)

Third-party libraries might be dynamically loaded from the system in unbundled configurations;
we should support unbundled libraries. This makes patching all dependencies unrealistic. A
solution to address such configuration would be to `LD_PRELOAD` an `::open` which we control
and which always sets `O_CLOEXEC`. The approach here tidies up what we expose to external
code.


- Benjamin


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


On May 15, 2018, 6:19 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67137/
> -----------------------------------------------------------
> 
> (Updated May 15, 2018, 6:19 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and James Peach.
> 
> 
> Bugs: MESOS-8917
>     https://issues.apache.org/jira/browse/MESOS-8917
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Avoided leaking file descriptors in Mesos containerizer.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp f25d90651ef32495c9161c3eaed8a327d1b2b926 
> 
> 
> Diff: https://reviews.apache.org/r/67137/diff/1/
> 
> 
> Testing
> -------
> 
> `make check`
> 
> I still need to confirm this patch in CI on a wider range of scenarios.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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