Page MenuHomePhabricator

Apply SOCK_CLOEXEC flag to socket API functions in order to avoid handle leakage to a forked child process.
ClosedPublic

Authored by ovyalov on Nov 10 2014, 3:21 PM.

Details

Reviewers
zturner
clayborg
Summary

Launch of a new process copies opened handles from parent process to child.
Setting SOCK_CLOEXEC/FD_CLOEXEC flag on kernel handle should prevent such kind of "resource sharing".
I made corresponding changes only for socket API - if such approach looks reasonable I'm planning to apply similar changes to files (source/Host/common/File.cpp) and pipes (source/Host/posix/PipePosix.cpp).

Diff Detail

Event Timeline

ovyalov updated this revision to Diff 16013.Nov 10 2014, 3:21 PM
ovyalov retitled this revision from to Apply SOCK_CLOEXEC flag to socket API functions in order to avoid handle leakage to a forked child process..
ovyalov updated this object.
ovyalov edited the test plan for this revision. (Show Details)
ovyalov added reviewers: clayborg, sbest.
ovyalov added a subscriber: Unknown Object (MLST).

Some google searching suggests that SOCK_CLOEXEC and FD_CLOEXEC are linux specific flags. If this is correct, can we put them behind #ifdef _LINUX instead of #ifdef SOCK_CLOEXEC? It makes it more clear to the reader when the alternate codepath will actually execute.

That said, I'm not sure that it's useful to have this be the default behavior. Dont' most people on posix platforms *expect* handles to be inherited by forked processes?

In D6204#4, @zturner wrote:

Some google searching suggests that SOCK_CLOEXEC and FD_CLOEXEC are linux specific flags. If this is correct, can we put them behind #ifdef _LINUX instead of #ifdef SOCK_CLOEXEC? It makes it more clear to the reader when the >alternate codepath will actually execute.

I see 2 reasons to use #ifdef SOCK_CLOEXEC here:

  1. SOCK_CLOEXEC is used not only by Linux - FreeBSD and OSX have this flag.
  2. Old Linux kernels may lack support of this flag.

That said, I'm not sure that it's useful to have this be the default behavior. Dont' most people on posix platforms *expect* handles to be inherited by forked processes?

Yes, it's expected behavior for forked processes but my understanding that in LLDB there is no need to share handles between processes (lldb and debugserver/lldb-gdbserver; debugserver/lldb-gdbserver and inferior) - such sharing may lead useless resource handles in child process and potential security issues when inferior has access to tracer's resources.

clayborg requested changes to this revision.Nov 10 2014, 4:45 PM
clayborg edited edge metadata.

I don't think this is a good default. Sockets are often used for IPC after fork/exec, but I would like to make a way to specify close on exec by supplying a boolean argument to ConnectionFileDescriptor() when creating it. Then clients will opt into this by force as they will need to supply the argument. Or you can make a default argument that defaults to true for "close_on_exec".

So maybe a boolean to:

virtual lldb::ConnectionStatus ConnectionFileDescriptor::Connect(const char *s, Error *error_ptr, bool close_on_exec = true);

Then this gets passed along to whomever creates the IOObject subclass and they each do the right thing if they can

This revision now requires changes to proceed.Nov 10 2014, 4:45 PM

Can I also request that the variable *not* be called close_on_exec?. It is
possible to implement an equivalent on Windows, but calling it close on
exec will be confusing to people on Windows. Can we come up with a more
generic name? On Windows this would be called "inherit handles", which to
me sounds like a better name than close on exec, but I may be biased. Is
"inherit handles" confusing to people on non-Windows?

In other words, I'm proposing

virtual lldb::ConnectionStatus ConnectionFileDescriptor::Connect(const char
*s, Error *error_ptr, bool close_on_exec = true);

would become

virtual lldb::ConnectionStatus ConnectionFileDescriptor::Connect(const char
*s, Error *error_ptr, bool inherit_handles = false);

In D6204#10, @zturner wrote:

Can I also request that the variable *not* be called close_on_exec?. It is
possible to implement an equivalent on Windows, but calling it close on
exec will be confusing to people on Windows. Can we come up with a more
generic name? On Windows this would be called "inherit handles", which to
me sounds like a better name than close on exec, but I may be biased. Is
"inherit handles" confusing to people on non-Windows?

In other words, I'm proposing

virtual lldb::ConnectionStatus ConnectionFileDescriptor::Connect(const char
*s, Error *error_ptr, bool close_on_exec = true);

would become

virtual lldb::ConnectionStatus ConnectionFileDescriptor::Connect(const char
*s, Error *error_ptr, bool inherit_handles = false);

It sounds good to me to call this variable inherit_handles in order to make it more platform-independent.
As I can see ConnectionFileDescriptor::Connect is overrides pure method from Connection::Connect (const char *url, Error *error_ptr) and I'm a bit reluctant to make such variable visible for Communication layer - so, I'm thinking about extending ConnectionFileDescriptor constructor in a way like this:

ConnectionFileDescriptor(int fd, bool owns_fd, bool inherit_handles = false);

In this case methods within ConnectionFileDescriptor::Connect (SocketListen, NamedSocketAccept, NamedSocketAccept, ConnectTCP, ConnectUDP) may use member field m_inherit_handles instead of taking additional argument.

I'm not sure it makes sense to put it on that constructor, because that
constructor takes an fd which has already been opened, so there would be no
way to set the flag on an already opened fd.

CFD has a default constructor though, it would probably make more sense on
that, and leave it off of the fd constructor.

Sounds good to me.

ovyalov updated this revision to Diff 16064.Nov 11 2014, 3:23 PM
ovyalov edited reviewers, added: zturner; removed: sbest.

Hi,

I've added a new field ConnectionFileDescriptor::m_child_processes_inherit and made socket factory methods to take Options structure as a parameter.
Please take another look and let me know your opinion.

Thank you!

zturner added inline comments.Nov 11 2014, 3:28 PM
include/lldb/Host/Socket.h
51

Can you fix the formatting here? LLDB uses

struct Options
{
};

Alternatively, it seems a little unnecessary to have this struct for only one boolean variable. You could just pass the option by value. Even if more options are added in the future, it could probably just be turned into a bitmask

source/Host/common/Socket.cpp
63

I think the #ifdef should be aligned with column 0. Same in the rest of them.

clayborg requested changes to this revision.Nov 11 2014, 3:45 PM
clayborg edited edge metadata.

I would vote to define the options struct in ConnectionFileDescriptor and have it not be an argument to the constructor, but to the connect method:

ConnectionFileDescriptor::Connect(const char *s, Error *error_ptr, const Options &options);

Make sure there is a default constructor for the Options class that sets m_child_processes_inherit to true;

And then pass the options from ConnectionFileDescriptor::Connect() down into all of the Socket::*Connect() calls

This revision now requires changes to proceed.Nov 11 2014, 3:45 PM
zturner edited edge metadata.Nov 11 2014, 3:53 PM
In D6204#18, @clayborg wrote:

I would vote to define the options struct in ConnectionFileDescriptor and have it not be an argument to the constructor, but to the connect method:

ConnectionFileDescriptor::Connect(const char *s, Error *error_ptr, const Options &options);

Make sure there is a default constructor for the Options class that sets m_child_processes_inherit to true;

And then pass the options from ConnectionFileDescriptor::Connect() down into all of the Socket::*Connect() calls

Wouldn't it be a little weird having a parameter on the Connect() method, which would then require modifying the base Connection class which is non-specific to files?

In D6204#21, @zturner wrote:
In D6204#18, @clayborg wrote:

I would vote to define the options struct in ConnectionFileDescriptor and have it not be an argument to the constructor, but to the connect method:

ConnectionFileDescriptor::Connect(const char *s, Error *error_ptr, const Options &options);

Make sure there is a default constructor for the Options class that sets m_child_processes_inherit to true;

And then pass the options from ConnectionFileDescriptor::Connect() down into all of the Socket::*Connect() calls

Wouldn't it be a little weird having a parameter on the Connect() method, which would then require modifying the base Connection class which is non-specific to files?

I agree with Zach that extending Connect() method in such way may expose file-specific details to Connection class dependencies. I see 2 unclear things for me here:

  • As I can see LLVM functions/methods follow the
In D6204#21, @zturner wrote:
In D6204#18, @clayborg wrote:

I would vote to define the options struct in ConnectionFileDescriptor and have it not be an argument to the constructor, but to the connect method:

ConnectionFileDescriptor::Connect(const char *s, Error *error_ptr, const Options &options);

Make sure there is a default constructor for the Options class that sets m_child_processes_inherit to true;

And then pass the options from ConnectionFileDescriptor::Connect() down into all of the Socket::*Connect() calls

Wouldn't it be a little weird having a parameter on the Connect() method, which would then require modifying the base Connection class which is non-specific to files?

I agree with Zach that extending Connect() method in such way may expose file-specific details to Connection class dependencies. Additionally there are a few unclear things for me here:

  • As I can see LLVM/LLDB functions/methods follow the convention where Error is always a last parameter if provided.
  • If options struct is defined in ConnectionFileDescriptor it means that this struct type should be visible to Socket. In this case we may have mutual referencing between Socket and ConnectionFileDescriptor.
  • If default constructor for the Options sets m_child_processes_inherit to true it means that we need to pass explicitly Options{false} at each ConnectionFileDescriptor::Connect call site, right?

Thank you.

In D6204#22, @ovyalov wrote:

I agree with Zach that extending Connect() method in such way may expose file-specific details to Connection class dependencies. Additionally there are a few unclear things for me here:

  • As I can see LLVM/LLDB functions/methods follow the convention where Error is always a last parameter if provided.
  • If options struct is defined in ConnectionFileDescriptor it means that this struct type should be visible to Socket. In this case we may have mutual referencing between Socket and ConnectionFileDescriptor.
  • If default constructor for the Options sets m_child_processes_inherit to true it means that we need to pass explicitly Options{false} at each ConnectionFileDescriptor::Connect call site, right?

Thank you.

I don't think we need an options struct at all. Why not just make it a bool?

Another, perhaps better option instead of passing it into the default constructor is to make a public method on CFD called SetChildProcessInherit(bool). This way you can set the value, call Connect, set the value to something else, call connect again, etc.

Since this inheritance is specific to files, anyone who wants to call this method already knows they're working with a ConnectionFileDescriptor, and not some other kind of Connection, so they already have a pointer or reference to a ConnectionFileDescriptor, and not the base class. So they can easily call this method. Then just pass it through as a bool, and not as an Options structure.

In D6204#23, @zturner wrote:
In D6204#22, @ovyalov wrote:

I agree with Zach that extending Connect() method in such way may expose file-specific details to Connection class dependencies. Additionally there are a few unclear things for me here:

  • As I can see LLVM/LLDB functions/methods follow the convention where Error is always a last parameter if provided.
  • If options struct is defined in ConnectionFileDescriptor it means that this struct type should be visible to Socket. In this case we may have mutual referencing between Socket and ConnectionFileDescriptor.
  • If default constructor for the Options sets m_child_processes_inherit to true it means that we need to pass explicitly Options{false} at each ConnectionFileDescriptor::Connect call site, right?

Thank you.

I don't think we need an options struct at all. Why not just make it a bool?

I'm okay with having bool variable in ConnectionFileDescriptor but then question arises how to pass this value to multiple Socket factory methods - TcpConnect, UdpConnect, TcpListen,... I'd rather pass child_processes_inherit to them as a part Options struct (or bit-masked int flags) - so, if we'll need to support a new option here (e.g., non-blocking IO) no need to modify Socket method signatures again.

Another, perhaps better option instead of passing it into the default constructor is to make a public method on CFD called SetChildProcessInherit(bool). This way you can set the value, call Connect, set the value to something else, call connect again, etc.

Since this inheritance is specific to files, anyone who wants to call this method already knows they're working with a >ConnectionFileDescriptor, and not some other kind of Connection, so they already have a pointer or reference to a >ConnectionFileDescriptor, and not the base class. So they can easily call this method. Then just pass it through as a bool, and not >as an Options structure.

Makes sense.

Lets just send bools for now in all calls and worry about extra stuff later.

ovyalov updated this revision to Diff 16178.Nov 13 2014, 2:04 PM
ovyalov edited edge metadata.

Replaced Options struct with bool variable. Please take another look.

Thank you!

clayborg accepted this revision.Nov 13 2014, 2:07 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Nov 13 2014, 2:07 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r222004.