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

[ovyalov]

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).

[zturner]

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?

[ovyalov]

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]

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

[zturner]

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);

[ovyalov]

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.

[zturner]

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.

[zturner]

Sounds good to me.

[ovyalov]

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]

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]

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

[zturner]

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?

[ovyalov]

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.

[zturner]

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.

[ovyalov]

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.

[clayborg]

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

[ovyalov]

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

Thank you!

[clayborg]

Looks good.

[Eugene.Zelenko]

Committed in r222004.

include/lldb/Host/Socket.h

         ProtocolUnixDomain
     } SocketProtocol;
 
+    struct Options {
+      bool m_child_processes_inherit;
+    };
+
     static const NativeSocket kInvalidSocketValue;
 
     Socket(NativeSocket socket, SocketProtocol protocol, bool should_close);

     // Initialize a Tcp Socket object in listening mode.  listen and accept are implemented
     // separately because the caller may wish to manipulate or query the socket after it is
     // initialized, but before entering a blocking accept.
-    static Error TcpListen(llvm::StringRef host_and_port, Socket *&socket, Predicate<uint16_t>* predicate);
-    static Error TcpConnect(llvm::StringRef host_and_port, Socket *&socket);
-    static Error UdpConnect(llvm::StringRef host_and_port, Socket *&send_socket, Socket *&recv_socket);
-    static Error UnixDomainConnect(llvm::StringRef host_and_port, Socket *&socket);
-    static Error UnixDomainAccept(llvm::StringRef host_and_port, Socket *&socket);
+    static Error TcpListen(llvm::StringRef host_and_port, const Options& opts, Socket *&socket, Predicate<uint16_t>* predicate);
+    static Error TcpConnect(llvm::StringRef host_and_port, const Options& opts, Socket *&socket);
+    static Error UdpConnect(llvm::StringRef host_and_port, const Options& opts, Socket *&send_socket, Socket *&recv_socket);
+    static Error UnixDomainConnect(llvm::StringRef host_and_port, const Options& opts, Socket *&socket);
+    static Error UnixDomainAccept(llvm::StringRef host_and_port, const Options& opts, Socket *&socket);
 
     // Blocks on a listening socket until a connection is received.  This method assumes that
     // |this->m_socket| is a listening socket, created via either TcpListen() or via the native
     // constructor that takes a NativeSocket, which itself was created via a call to |listen()|
-    Error BlockingAccept(llvm::StringRef host_and_port, Socket *&socket);
+    Error BlockingAccept(llvm::StringRef host_and_port, const Options& opts, Socket *&socket);
 
     int GetOption (int level, int option_name, int &option_value);
     int SetOption (int level, int option_name, int option_value);

include/lldb/Host/posix/ConnectionFileDescriptorPosix.h

 class ConnectionFileDescriptor : public Connection
 {
   public:
-    ConnectionFileDescriptor();
+    ConnectionFileDescriptor(bool child_processes_inherit = false);
 
     ConnectionFileDescriptor(int fd, bool owns_fd);
 

     std::atomic<bool> m_shutting_down; // This marks that we are shutting down so if we get woken up from
                                        // BytesAvailable to disconnect, we won't try to read again.
     bool m_waiting_for_accept;
+    bool m_child_processes_inherit;
 
   private:
     DISALLOW_COPY_AND_ASSIGN(ConnectionFileDescriptor);

source/Host/common/Socket.cpp

 #endif
 #endif // #ifdef __ANDROID__
 
+namespace {
+
+NativeSocket CreateSocket(const int domain, const int type, const int protocol, const Socket::Options& opts)
+{
+  auto socketType = type;
+  #ifdef SOCK_CLOEXEC
+  if (!opts.m_child_processes_inherit) {
+    socketType |= SOCK_CLOEXEC;
+  }
+  #endif
+  return ::socket (domain, socketType, protocol);
+}
+
+NativeSocket Accept(NativeSocket sockfd, struct sockaddr *addr, socklen_t *addrlen, const Socket::Options& opts)
+{
+  #ifdef SOCK_CLOEXEC
+  int flags = 0;
+  if (!opts.m_child_processes_inherit) {
+    flags |= SOCK_CLOEXEC;
+  }
+  return ::accept4 (sockfd, addr, addrlen, flags);
+  #else
+  return ::accept (sockfd, addr, addrlen);
+  #endif
+}
+}
+
 Socket::Socket(NativeSocket socket, SocketProtocol protocol, bool should_close)
     : IOObject(eFDTypeSocket, should_close)
     , m_protocol(protocol)

     Close();
 }
 
-Error Socket::TcpConnect(llvm::StringRef host_and_port, Socket *&socket)
+Error Socket::TcpConnect(llvm::StringRef host_and_port, const Options& opts, Socket *&socket)
 {
     // Store the result in a unique_ptr in case we error out, the memory will get correctly freed.
     std::unique_ptr<Socket> final_socket;

         return error;
 
     // Create the socket
-    sock = ::socket (AF_INET, SOCK_STREAM, IPPROTO_TCP);
+    sock = CreateSocket (AF_INET, SOCK_STREAM, IPPROTO_TCP, opts);
     if (sock == kInvalidSocketValue)
     {
         // TODO: On Windows, use WSAGetLastError().

     return error;
 }
 
-Error Socket::TcpListen(llvm::StringRef host_and_port, Socket *&socket, Predicate<uint16_t>* predicate)
+Error Socket::TcpListen(llvm::StringRef host_and_port, const Options& opts, Socket *&socket, Predicate<uint16_t>* predicate)
 {
     std::unique_ptr<Socket> listen_socket;
     NativeSocket listen_sock = kInvalidSocketValue;

     const sa_family_t family = AF_INET;
     const int socktype = SOCK_STREAM;
     const int protocol = IPPROTO_TCP;
-    listen_sock = ::socket (family, socktype, protocol);
+    listen_sock = ::CreateSocket (family, socktype, protocol, opts);
     if (listen_sock == kInvalidSocketValue)
     {
         error.SetErrorToErrno();

     return error;
 }
 
-Error Socket::BlockingAccept(llvm::StringRef host_and_port, Socket *&socket)
+Error Socket::BlockingAccept(llvm::StringRef host_and_port, const Options& opts, Socket *&socket)
 {
     Error error;
     std::string host_str;

 #endif
         socklen_t accept_addr_len = sizeof accept_addr;
 
-        int sock = ::accept (this->GetNativeSocket(), (struct sockaddr *)&accept_addr, &accept_addr_len);
+        int sock = Accept (this->GetNativeSocket(), (struct sockaddr *)&accept_addr, &accept_addr_len, opts);
             
         if (sock == kInvalidSocketValue)
         {

 
 }
 
-Error Socket::UdpConnect(llvm::StringRef host_and_port, Socket *&send_socket, Socket *&recv_socket)
+Error Socket::UdpConnect(llvm::StringRef host_and_port, const Options& opts, Socket *&send_socket, Socket *&recv_socket)
 {
     std::unique_ptr<Socket> final_send_socket;
     std::unique_ptr<Socket> final_recv_socket;

 
     // Setup the receiving end of the UDP connection on this localhost
     // on port zero. After we bind to port zero we can read the port.
-    final_recv_fd = ::socket (AF_INET, SOCK_DGRAM, 0);
+    final_recv_fd = ::CreateSocket (AF_INET, SOCK_DGRAM, 0, opts);
     if (final_recv_fd == kInvalidSocketValue)
     {
         // Socket creation failed...

          service_info_ptr != NULL; 
          service_info_ptr = service_info_ptr->ai_next) 
     {
-        final_send_fd = ::socket (service_info_ptr->ai_family, 
+        final_send_fd = ::CreateSocket (service_info_ptr->ai_family,
                                         service_info_ptr->ai_socktype,
-                                  service_info_ptr->ai_protocol);
+                                        service_info_ptr->ai_protocol,
+                                        opts);
 
         if (final_send_fd != kInvalidSocketValue)
         {

     return error;
 }
 
-Error Socket::UnixDomainConnect(llvm::StringRef name, Socket *&socket)
+Error Socket::UnixDomainConnect(llvm::StringRef name, const Options& opts, Socket *&socket)
 {
     Error error;
 #ifndef LLDB_DISABLE_POSIX

 
     // Open the socket that was passed in as an option
     struct sockaddr_un saddr_un;
-    int fd = ::socket (AF_UNIX, SOCK_STREAM, 0);
+    int fd = ::CreateSocket (AF_UNIX, SOCK_STREAM, 0, opts);
     if (fd == kInvalidSocketValue)
     {
         error.SetErrorToErrno();

     return error;
 }
 
-Error Socket::UnixDomainAccept(llvm::StringRef name, Socket *&socket)
+Error Socket::UnixDomainAccept(llvm::StringRef name, const Options& opts, Socket *&socket)
 {
     Error error;
 #ifndef LLDB_DISABLE_POSIX

     NativeSocket listen_fd = kInvalidSocketValue;
     NativeSocket socket_fd = kInvalidSocketValue;
     
-    listen_fd = ::socket (AF_UNIX, SOCK_STREAM, 0);
+    listen_fd = ::CreateSocket (AF_UNIX, SOCK_STREAM, 0, opts);
     if (listen_fd == kInvalidSocketValue)
     {
         error.SetErrorToErrno();

     {
         if (::listen (listen_fd, 5) == 0) 
         {
-            socket_fd = ::accept (listen_fd, NULL, 0);
+            socket_fd = Accept (listen_fd, NULL, 0, opts);
             if (socket_fd > 0)
             {
                 final_socket.reset(new Socket(socket_fd, ProtocolUnixDomain, true));

source/Host/posix/ConnectionFileDescriptorPosix.cpp

 using namespace lldb;
 using namespace lldb_private;
 
-ConnectionFileDescriptor::ConnectionFileDescriptor()
+ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit)
     : Connection()
     , m_pipe()
     , m_mutex(Mutex::eMutexTypeRecursive)
     , m_shutting_down(false)
     , m_waiting_for_accept(false)
+    , m_child_processes_inherit(child_processes_inherit)
 {
     Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION | LIBLLDB_LOG_OBJECT));
     if (log)

     , m_mutex(Mutex::eMutexTypeRecursive)
     , m_shutting_down(false)
     , m_waiting_for_accept(false)
+    , m_child_processes_inherit(false)
 {
     m_write_sp.reset(new File(fd, owns_fd));
     m_read_sp.reset(new File(fd, false));

 ConnectionFileDescriptor::NamedSocketAccept(const char *socket_name, Error *error_ptr)
 {
     Socket *socket = nullptr;
-    Error error = Socket::UnixDomainAccept(socket_name, socket);
+    Error error = Socket::UnixDomainAccept(socket_name, {m_child_processes_inherit}, socket);
     if (error_ptr)
         *error_ptr = error;
     m_write_sp.reset(socket);

 ConnectionFileDescriptor::NamedSocketConnect(const char *socket_name, Error *error_ptr)
 {
     Socket *socket = nullptr;
-    Error error = Socket::UnixDomainConnect(socket_name, socket);
+    Error error = Socket::UnixDomainConnect(socket_name, {m_child_processes_inherit}, socket);
     if (error_ptr)
         *error_ptr = error;
     m_write_sp.reset(socket);

 
     Socket *socket = nullptr;
     m_waiting_for_accept = true;
-    Error error = Socket::TcpListen(s, socket, &m_port_predicate);
+    Error error = Socket::TcpListen(s, {m_child_processes_inherit}, socket, &m_port_predicate);
     if (error_ptr)
         *error_ptr = error;
     if (error.Fail())

 
     listening_socket_up.reset(socket);
     socket = nullptr;
-    error = listening_socket_up->BlockingAccept(s, socket);
+    error = listening_socket_up->BlockingAccept(s, {m_child_processes_inherit}, socket);
     listening_socket_up.reset();
     if (error_ptr)
         *error_ptr = error;

 ConnectionFileDescriptor::ConnectTCP(const char *s, Error *error_ptr)
 {
     Socket *socket = nullptr;
-    Error error = Socket::TcpConnect(s, socket);
+    Error error = Socket::TcpConnect(s, {m_child_processes_inherit}, socket);
     if (error_ptr)
         *error_ptr = error;
     m_write_sp.reset(socket);

 {
     Socket *send_socket = nullptr;
     Socket *recv_socket = nullptr;
-    Error error = Socket::UdpConnect(s, send_socket, recv_socket);
+    Error error = Socket::UdpConnect(s, {m_child_processes_inherit}, send_socket, recv_socket);
     if (error_ptr)
         *error_ptr = error;
     m_write_sp.reset(send_socket);