Skip to content

Commit

Permalink
fd_restrict prototype
Browse files Browse the repository at this point in the history
  • Loading branch information
@sehe
sehe committed Jun 2, 2017
1 parent 4075371 commit 3d1f77b
Show file tree
Hide file tree
Showing 4 changed files with 178 additions and 2 deletions.
10 changes: 9 additions & 1 deletion include/boost/process/detail/posix/executor.hpp
View file
Expand Up @@ -15,6 +15,7 @@
#include <boost/process/pipe.hpp>
#include <boost/process/detail/posix/basic_pipe.hpp>
#include <boost/process/detail/posix/use_vfork.hpp>
#include <boost/process/detail/posix/file_descriptor.hpp>
#include <boost/fusion/algorithm/iteration/for_each.hpp>
#include <cstdlib>
#include <sys/types.h>
Expand Down Expand Up @@ -329,6 +330,12 @@ class executor
}
void set_error(const std::error_code &ec, const std::string &msg) {set_error(ec, msg.c_str());};

// customization point for fd_restrict
template <typename OutputIterator>
friend void collect_filedescriptors(executor const& me, OutputIterator& outit) {
// always protect the write end of the parent/child pipe
*outit++ = me._pipe_sink;
}
};

template<typename Sequence>
Expand Down Expand Up @@ -379,6 +386,7 @@ child executor<Sequence>::invoke(boost::mpl::false_, boost::mpl::false_)
return child();
}
_ec.clear();
_pipe_sink = p[1];
boost::fusion::for_each(seq, call_on_setup(*this));

if (_ec)
Expand All @@ -390,6 +398,7 @@ child executor<Sequence>::invoke(boost::mpl::false_, boost::mpl::false_)
this->pid = ::fork();
if (pid == -1)
{
_pipe_sink = -1;
_ec = boost::process::detail::get_last_error();
_msg = "fork() failed";
boost::fusion::for_each(seq, call_on_fork_error(*this, _ec));
Expand All @@ -399,7 +408,6 @@ child executor<Sequence>::invoke(boost::mpl::false_, boost::mpl::false_)
}
else if (pid == 0)
{
_pipe_sink = p[1];
::close(p[0]);

boost::fusion::for_each(seq, call_on_exec_setup(*this));
Expand Down
9 changes: 8 additions & 1 deletion include/boost/process/detail/posix/fd.hpp
View file
Expand Up @@ -11,6 +11,7 @@
#define BOOST_PROCESS_DETAIL_POSIX_FD_HPP

#include <boost/process/detail/posix/handler.hpp>
#include <boost/process/detail/posix/fd_restrict.hpp>
#include <unistd.h>

namespace boost { namespace process { namespace detail { namespace posix {
Expand Down Expand Up @@ -68,11 +69,16 @@ struct bind_fd_ : handler_base_ext
}

private:
// customization point for fd_restrict
template <typename OutputIterator>
friend void collect_filedescriptors(bind_fd_ const& bind_fd, OutputIterator& outit) {
*outit++ = bind_fd.id_;
}

int id_;
FileDescriptor fd_;
};


struct fd_
{
constexpr fd_() {};
Expand All @@ -84,6 +90,7 @@ struct fd_
template <class FileDescriptor>
bind_fd_<FileDescriptor> bind(int id, const FileDescriptor & fd) const {return {id, fd};}

fd_restrict::property_<> restrict_inherit(size_t capacity = 128) const {return {capacity};}
};


Expand Down
154 changes: 154 additions & 0 deletions include/boost/process/detail/posix/fd_restrict.hpp
View file
@@ -0,0 +1,154 @@
// Copyright (c) 2017 Seth Heeren
//
// Distributed under the Boost Software License, Version 1.0. (See accompanying
// file LICENSE_1_0.txt or copy at http://www.boost.org/LICENSE_1_0.txt)

#ifndef BOOST_PROCESS_DETAIL_POSIX_FD_RESTRICT_HPP
#define BOOST_PROCESS_DETAIL_POSIX_FD_RESTRICT_HPP

#include <boost/process/detail/posix/handler.hpp>
#include <unistd.h>

namespace boost { namespace process { namespace detail { namespace posix { namespace fd_restrict {
// customization point for (custom) properties that need to protect fds
template <typename Property, typename OutputIterator>
void collect_filedescriptors(Property const& /*property*/, OutputIterator& /*outit*/) {
// primary template
}

// polymorphic function object for ADL dispatch
template <typename OutputIterator>
struct collect_fd_f {
OutputIterator mutable _outit;

template <typename Property>
void operator()(Property const& property) const {
using boost::process::detail::posix::fd_restrict::collect_filedescriptors; // ADL desired
collect_filedescriptors(property, _outit);
}
};

// launch property
template <typename=void>
struct property_ : handler_base_ext
{
public:
property_(size_t capacity) {
// reserve to avoid allocations between fork/exec which may
// deadlock with threads
_protected_fds.reserve(capacity);
}

template <class PosixExecutor>
void on_exec_setup(PosixExecutor& exec) const
{
_protected_fds.resize(0);
auto outit = back_inserter(_protected_fds);
collect_fd_f<decltype(outit)> visit{outit};

visit(exec);
boost::fusion::for_each(exec.seq, visit);

auto begin = _protected_fds.begin(), end = _protected_fds.end();
std::sort(begin, end);

for(int fd=0, maxfd=sysconf(_SC_OPEN_MAX); fd<maxfd; ++fd) {

This comment has been minimized.

Sign in to view

Copy link
@sehe

sehe Jun 3, 2017

Author Owner

There are many contradictory articles about portably closing all FDs on POSIX. This brute-force approach appears to be the only one guaranteed.

(One implication is that fd.close() and friends could still be worthwhile in case you wanted error-checked close of a particular fd.)
if (!std::binary_search(begin, end, fd))
::close(fd);
}
}

private:
std::vector<int> mutable _protected_fds;
};

}}}}}

/*
* Non-intrusive instrumentation of existing POSIX properties that require filedescriptors
*
* All of these could be done with an inline `friend` definition, like above.
*
* For now I prefer to keep them separate so that
*
* - upstream changes merge cleanly
* - interface changes in fd_restrict can more easily be applied consistently in 1 file
*
* Only bind_fd_ and filedescriptor need friend access, so cannot usefully be kept separate.
*/

#include <boost/process/detail/posix/async_in.hpp>
#include <boost/process/detail/posix/async_out.hpp>
#include <boost/process/detail/posix/null_in.hpp>
#include <boost/process/detail/posix/null_out.hpp>
#include <boost/process/detail/posix/file_in.hpp>
#include <boost/process/detail/posix/file_out.hpp>
#include <boost/process/detail/posix/pipe_in.hpp>
#include <boost/process/detail/posix/pipe_out.hpp>

namespace boost { namespace process { namespace detail { namespace posix {

template<typename... Ts, typename OutputIterator>
void collect_filedescriptors(async_in_buffer<Ts...> const&, OutputIterator& outit) {
*outit++ = STDIN_FILENO;
}

template<int p1, int p2, typename... Ts, typename OutputIterator>
void collect_filedescriptors(async_out_buffer<p1, p2, Ts...> const&, OutputIterator& outit) {
if (p1==1||p2==1) *outit++ = STDOUT_FILENO;
if (p1==2||p2==2) *outit++ = STDERR_FILENO;
}

template<int p1, int p2, typename... Ts, typename OutputIterator>
void collect_filedescriptors(async_out_future<p1, p2, Ts...> const&, OutputIterator& outit) {
if (p1==1||p2==1) *outit++ = STDOUT_FILENO;
if (p1==2||p2==2) *outit++ = STDERR_FILENO;
}

template<typename OutputIterator>
void collect_filedescriptors(file_in const&, OutputIterator& outit) {
*outit++ = STDIN_FILENO;
}

template<int p1, int p2, typename OutputIterator>
void collect_filedescriptors(file_out<p1, p2> const&, OutputIterator& outit) {
if (p1==1||p2==1) *outit++ = STDOUT_FILENO;
if (p1==2||p2==2) *outit++ = STDERR_FILENO;
}

template<typename OutputIterator>
void collect_filedescriptors(null_in const&, OutputIterator& outit) {
*outit++ = STDIN_FILENO;
}

template<int p1, int p2, typename OutputIterator>
void collect_filedescriptors(null_out<p1, p2> const&, OutputIterator& outit) {
if (p1==1||p2==1) *outit++ = STDOUT_FILENO;
if (p1==2||p2==2) *outit++ = STDERR_FILENO;
}

template<typename OutputIterator>
void collect_filedescriptors(pipe_in const&, OutputIterator& outit) {
*outit++ = STDIN_FILENO;
}

template<typename OutputIterator>
void collect_filedescriptors(async_pipe_in const&, OutputIterator& outit) {
*outit++ = STDIN_FILENO;
}

template<int p1, int p2, typename OutputIterator>
void collect_filedescriptors(pipe_out<p1, p2> const&, OutputIterator& outit) {
if (p1==1||p2==1) *outit++ = STDOUT_FILENO;
if (p1==2||p2==2) *outit++ = STDERR_FILENO;
}

template<int p1, int p2, typename OutputIterator>
void collect_filedescriptors(async_pipe_out<p1, p2> const&, OutputIterator& outit) {
if (p1==1||p2==1) *outit++ = STDOUT_FILENO;
if (p1==2||p2==2) *outit++ = STDERR_FILENO;
}

}}}}

#endif
7 changes: 7 additions & 0 deletions include/boost/process/detail/posix/file_descriptor.hpp
View file
Expand Up @@ -53,6 +53,13 @@ struct file_descriptor
int handle() const { return _handle;}

private:
// customization point for fd_restrict
template <typename OutputIterator>
friend void collect_filedescriptors(file_descriptor const& property_, OutputIterator& outit) {
if (-1 != property_._handle)
*outit++ = property_._handle;
}

static int create_file(const char* name, mode_t mode )
{
switch(mode)
Expand Down

9 comments on commit 3d1f77b

@sehe
Copy link
Owner Author

@sehe sehe commented on 3d1f77b Jun 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a thing. It does WorkForMe™.

It adds a fd_restrict namespace + ditto launch property that can be added via bp::posix::fg.restrict_inherit(). If it is specified, only expressly fds expressly required by any Boost Process feature are inherited, and the rest is closed.

The fd collection mechanism is extensible using an ADL-enabled customization point (collect_filedescriptors). For now, the integration with existing POSIX properties is done non-intrusively when possible, inside the fd_restrict.hpp header.

Note that the actual action of closing is done in the on_setup_exec of the fd_restrict property. It would make the most sense to add this as the last, in case there are inter-dependencies (e.g. other launch properties still require information from open descriptors before execve.

Bikeshed: The interface may be a bit random. I find it hard to come up with a consistent interface given the presence of "negative" inheriters (bp::posix::fd.close(fd) e.g.) and "inherit by default". If you're open to changing those defaults, things could be more natural, and more secure(!).

I have done some testing in our codebase, but the tests don't transfer easily. I'll probably find some time to write tests for the repo.

@klemens-morgenstern
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sort of works, but it's not extensible enough for me. How about letting every intializer like file_in inherit a carries_fd type, so I can filter that by type_traits and thus extend it more easily.

@klemens-morgenstern
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition you could also count the elements containing file-descriptors at runtime, thus saving an allocation - or having none at all by using std::array. Not that this is major, but if it's possible...

@sehe
Copy link
Owner Author

@sehe sehe commented on 3d1f77b Jun 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sort of works, but it's not extensible enough for me. How about letting every intializer like file_in inherit a carries_fd type, so I can filter that by type_traits and thus extend it more easily.

carries_fd logically doesn't convey which fd, so you parameterize it with the fd that's actually "carried". I I actually explored this option. It turns out to be inconvenient

  • because you might carry more than one fd. (e.g. all the current output properties)
  • if the FD is not statically known (see posix executor, file_descriptor, bind_fd_ e.g.) you'll still need to have a dynamic interface to get at those values.

Therefore having a carries_fd trait appears to strictly make things harder. You still have to make it return specific fds (which becomes clumsy for non-static, in effect other than stdin/stdout/stderr) as well as creating the marker typedef.

Summary

I'd like to note that the current setup requires literally ZERO effort to support, because of the default implementation of collect_filedescriptors.

  • I'm open to renaming it to carried_fds or similar
  • Yes, we could reduce some code duplication using traits only for standard filedescriptor handles. That just requires some intrusive changes which I specifically wanted to avoid (because I need to support this patch locally for indefinite time.)

PS. Note also that the current customization point transparently applies to the executor itself.

If I misunderstood your suggestion, perhaps you can show me some (pseudo) code.

@sehe
Copy link
Owner Author

@sehe sehe commented on 3d1f77b Jun 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition you could also count the elements containing file-descriptors at runtime, thus saving an allocation - or having none at all by using std::array. Not that this is major, but if it's possible...

I aimed for genericity. We can ONLY count things if we assume that the number of fds carried by (custom) properties is fixed. And we can ONLY use fixed size containers if those numbers are statically known. (Worse, if it's dynamically known, custom properties might not know the actual number pre-fork).

The only thing I would deem reasonably to add here is protection against inadvertent reallocation (the output iterator doesn't prevent or detect it). This begins to seem like overkill in the light of klemens-morgenstern#65 (comment).

I'd be happy to replace the output iterator by a callback that can actually refuse if capacity is exceeded. This would be a splendid middle-ground IYAM. I admit that I was a little lazy there (or in Proof-Of-Concept mode).

@klemens-morgenstern
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you're right, and for whatever reason a custom initializer might have more than one handle. But that doesn't mean we can't tag them, here's a what I came up with: https://godbolt.org/g/nzllVe

@sraza1
Copy link

@sraza1 sraza1 commented on 3d1f77b Oct 23, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sehe
Did this patch set make into the mainline boost process module ?

@sehe
Copy link
Owner Author

@sehe sehe commented on 3d1f77b Oct 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sraza1 nope. See this post for the lastest status update I have

https://stackoverflow.com/questions/52357635/prevent-child-process-from-inheriting-parent-processs-opened-tcp-ports-with-boo/52358874#52358874

Short answer: it's untested, might not work. I have dropped Boost Process in favour of hand written code once again, because it was too hard to reason about whether bugs were being caused by me trying to use it.

@klemens-morgenstern
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard problem to solve as well - since it should be done portably, so what do we do on windows? The current solution just goes by the OS defaults. @sehe, if I give you the function to collect all the used fds, would that help? Or what other hand-coded things are needed?

Btw. if you know what descriptors should not be used, you can do this.

#include <boost/process/posix.hpp>

void foo()
{
    namespace bp = boost::process;
    bp::child proc{"foo", bp::posix::fd.close({1,3,5,42}));
}

Please sign in to comment.