[lldb] Remove child_process_inherit from the socket classes #117699
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Pavel Labath (labath) ChangesIt's never set to true. Also, using inheritable FDs in a multithreaded process pretty much guarantees descriptor leaks. It's better to explicitly pass a specific FD to a specific subprocess, which we already mostly can do using the ProcessLaunchInfo FileActions. Patch is 31.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/117699.diff 18 Files Affected:
diff --git a/lldb/include/lldb/Host/Socket.h b/lldb/include/lldb/Host/Socket.h
index 14468c98ac5a3a..06bd929818c559 100644
--- a/lldb/include/lldb/Host/Socket.h
+++ b/lldb/include/lldb/Host/Socket.h
@@ -93,7 +93,6 @@ class Socket : public IOObject {
static void Terminate();
static std::unique_ptr<Socket> Create(const SocketProtocol protocol,
- bool child_processes_inherit,
Status &error);
virtual Status Connect(llvm::StringRef name) = 0;
@@ -114,14 +113,13 @@ class Socket : public IOObject {
// implemented separately because the caller may wish to manipulate or query
// the socket after it is initialized, but before entering a blocking accept.
static llvm::Expected<std::unique_ptr<TCPSocket>>
- TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
- int backlog = 5);
+ TcpListen(llvm::StringRef host_and_port, int backlog = 5);
static llvm::Expected<std::unique_ptr<Socket>>
- TcpConnect(llvm::StringRef host_and_port, bool child_processes_inherit);
+ TcpConnect(llvm::StringRef host_and_port);
static llvm::Expected<std::unique_ptr<UDPSocket>>
- UdpConnect(llvm::StringRef host_and_port, bool child_processes_inherit);
+ UdpConnect(llvm::StringRef host_and_port);
static int GetOption(NativeSocket sockfd, int level, int option_name,
int &option_value);
@@ -153,8 +151,7 @@ class Socket : public IOObject {
virtual std::string GetRemoteConnectionURI() const { return ""; };
protected:
- Socket(SocketProtocol protocol, bool should_close,
- bool m_child_process_inherit);
+ Socket(SocketProtocol protocol, bool should_close);
virtual size_t Send(const void *buf, const size_t num_bytes);
@@ -162,15 +159,12 @@ class Socket : public IOObject {
static Status GetLastError();
static void SetLastError(Status &error);
static NativeSocket CreateSocket(const int domain, const int type,
- const int protocol,
- bool child_processes_inherit, Status &error);
+ const int protocol, Status &error);
static NativeSocket AcceptSocket(NativeSocket sockfd, struct sockaddr *addr,
- socklen_t *addrlen,
- bool child_processes_inherit, Status &error);
+ socklen_t *addrlen, Status &error);
SocketProtocol m_protocol;
NativeSocket m_socket;
- bool m_child_processes_inherit;
bool m_should_close_fd;
};
diff --git a/lldb/include/lldb/Host/common/TCPSocket.h b/lldb/include/lldb/Host/common/TCPSocket.h
index eefe0240fe4a95..ca36622691fe9a 100644
--- a/lldb/include/lldb/Host/common/TCPSocket.h
+++ b/lldb/include/lldb/Host/common/TCPSocket.h
@@ -17,9 +17,8 @@
namespace lldb_private {
class TCPSocket : public Socket {
public:
- TCPSocket(bool should_close, bool child_processes_inherit);
- TCPSocket(NativeSocket socket, bool should_close,
- bool child_processes_inherit);
+ explicit TCPSocket(bool should_close);
+ TCPSocket(NativeSocket socket, bool should_close);
~TCPSocket() override;
// returns port number or 0 if error
diff --git a/lldb/include/lldb/Host/common/UDPSocket.h b/lldb/include/lldb/Host/common/UDPSocket.h
index 7348010d02ada2..63012d4fb6025f 100644
--- a/lldb/include/lldb/Host/common/UDPSocket.h
+++ b/lldb/include/lldb/Host/common/UDPSocket.h
@@ -14,10 +14,10 @@
namespace lldb_private {
class UDPSocket : public Socket {
public:
- UDPSocket(bool should_close, bool child_processes_inherit);
+ explicit UDPSocket(bool should_close);
static llvm::Expected<std::unique_ptr<UDPSocket>>
- Connect(llvm::StringRef name, bool child_processes_inherit);
+ CreateConnected(llvm::StringRef name);
std::string GetRemoteConnectionURI() const override;
diff --git a/lldb/include/lldb/Host/linux/AbstractSocket.h b/lldb/include/lldb/Host/linux/AbstractSocket.h
index 78a567a6b90953..accfd01457a5e0 100644
--- a/lldb/include/lldb/Host/linux/AbstractSocket.h
+++ b/lldb/include/lldb/Host/linux/AbstractSocket.h
@@ -14,7 +14,7 @@
namespace lldb_private {
class AbstractSocket : public DomainSocket {
public:
- AbstractSocket(bool child_processes_inherit);
+ AbstractSocket();
protected:
size_t GetNameOffset() const override;
diff --git a/lldb/include/lldb/Host/posix/DomainSocket.h b/lldb/include/lldb/Host/posix/DomainSocket.h
index 983f43bd633719..d4e0d43ee169c1 100644
--- a/lldb/include/lldb/Host/posix/DomainSocket.h
+++ b/lldb/include/lldb/Host/posix/DomainSocket.h
@@ -14,9 +14,8 @@
namespace lldb_private {
class DomainSocket : public Socket {
public:
- DomainSocket(NativeSocket socket, bool should_close,
- bool child_processes_inherit);
- DomainSocket(bool should_close, bool child_processes_inherit);
+ DomainSocket(NativeSocket socket, bool should_close);
+ explicit DomainSocket(bool should_close);
Status Connect(llvm::StringRef name) override;
Status Listen(llvm::StringRef name, int backlog) override;
@@ -29,7 +28,7 @@ class DomainSocket : public Socket {
std::string GetRemoteConnectionURI() const override;
protected:
- DomainSocket(SocketProtocol protocol, bool child_processes_inherit);
+ DomainSocket(SocketProtocol protocol);
virtual size_t GetNameOffset() const;
virtual void DeleteSocketFile(llvm::StringRef name);
diff --git a/lldb/source/Host/common/Socket.cpp b/lldb/source/Host/common/Socket.cpp
index d69eb608204033..60dae8213068a0 100644
--- a/lldb/source/Host/common/Socket.cpp
+++ b/lldb/source/Host/common/Socket.cpp
@@ -180,12 +180,9 @@ bool Socket::FindProtocolByScheme(const char *scheme,
return false;
}
-Socket::Socket(SocketProtocol protocol, bool should_close,
- bool child_processes_inherit)
+Socket::Socket(SocketProtocol protocol, bool should_close)
: IOObject(eFDTypeSocket), m_protocol(protocol),
- m_socket(kInvalidSocketValue),
- m_child_processes_inherit(child_processes_inherit),
- m_should_close_fd(should_close) {}
+ m_socket(kInvalidSocketValue), m_should_close_fd(should_close) {}
Socket::~Socket() { Close(); }
@@ -214,24 +211,21 @@ void Socket::Terminate() {
}
std::unique_ptr<Socket> Socket::Create(const SocketProtocol protocol,
- bool child_processes_inherit,
Status &error) {
error.Clear();
+ const bool should_close = true;
std::unique_ptr<Socket> socket_up;
switch (protocol) {
case ProtocolTcp:
- socket_up =
- std::make_unique<TCPSocket>(true, child_processes_inherit);
+ socket_up = std::make_unique<TCPSocket>(should_close);
break;
case ProtocolUdp:
- socket_up =
- std::make_unique<UDPSocket>(true, child_processes_inherit);
+ socket_up = std::make_unique<UDPSocket>(should_close);
break;
case ProtocolUnixDomain:
#if LLDB_ENABLE_POSIX
- socket_up =
- std::make_unique<DomainSocket>(true, child_processes_inherit);
+ socket_up = std::make_unique<DomainSocket>(should_close);
#else
error = Status::FromErrorString(
"Unix domain sockets are not supported on this platform.");
@@ -239,8 +233,7 @@ std::unique_ptr<Socket> Socket::Create(const SocketProtocol protocol,
break;
case ProtocolUnixAbstract:
#ifdef __linux__
- socket_up =
- std::make_unique<AbstractSocket>(child_processes_inherit);
+ socket_up = std::make_unique<AbstractSocket>();
#else
error = Status::FromErrorString(
"Abstract domain sockets are not supported on this platform.");
@@ -255,14 +248,12 @@ std::unique_ptr<Socket> Socket::Create(const SocketProtocol protocol,
}
llvm::Expected<std::unique_ptr<Socket>>
-Socket::TcpConnect(llvm::StringRef host_and_port,
- bool child_processes_inherit) {
+Socket::TcpConnect(llvm::StringRef host_and_port) {
Log *log = GetLog(LLDBLog::Connection);
LLDB_LOG(log, "host_and_port = {0}", host_and_port);
Status error;
- std::unique_ptr<Socket> connect_socket(
- Create(ProtocolTcp, child_processes_inherit, error));
+ std::unique_ptr<Socket> connect_socket = Create(ProtocolTcp, error);
if (error.Fail())
return error.ToError();
@@ -274,13 +265,12 @@ Socket::TcpConnect(llvm::StringRef host_and_port,
}
llvm::Expected<std::unique_ptr<TCPSocket>>
-Socket::TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
- int backlog) {
+Socket::TcpListen(llvm::StringRef host_and_port, int backlog) {
Log *log = GetLog(LLDBLog::Connection);
LLDB_LOG(log, "host_and_port = {0}", host_and_port);
std::unique_ptr<TCPSocket> listen_socket(
- new TCPSocket(true, child_processes_inherit));
+ new TCPSocket(/*should_close=*/true));
Status error = listen_socket->Listen(host_and_port, backlog);
if (error.Fail())
@@ -290,9 +280,8 @@ Socket::TcpListen(llvm::StringRef host_and_port, bool child_processes_inherit,
}
llvm::Expected<std::unique_ptr<UDPSocket>>
-Socket::UdpConnect(llvm::StringRef host_and_port,
- bool child_processes_inherit) {
- return UDPSocket::Connect(host_and_port, child_processes_inherit);
+Socket::UdpConnect(llvm::StringRef host_and_port) {
+ return UDPSocket::CreateConnected(host_and_port);
}
llvm::Expected<Socket::HostAndPort> Socket::DecodeHostAndPort(llvm::StringRef host_and_port) {
@@ -445,13 +434,11 @@ int Socket::CloseSocket(NativeSocket sockfd) {
}
NativeSocket Socket::CreateSocket(const int domain, const int type,
- const int protocol,
- bool child_processes_inherit, Status &error) {
+ const int protocol, Status &error) {
error.Clear();
auto socket_type = type;
#ifdef SOCK_CLOEXEC
- if (!child_processes_inherit)
- socket_type |= SOCK_CLOEXEC;
+ socket_type |= SOCK_CLOEXEC;
#endif
auto sock = ::socket(domain, socket_type, protocol);
if (sock == kInvalidSocketValue)
@@ -474,8 +461,7 @@ Status Socket::Accept(Socket *&socket) {
}
NativeSocket Socket::AcceptSocket(NativeSocket sockfd, struct sockaddr *addr,
- socklen_t *addrlen,
- bool child_processes_inherit, Status &error) {
+ socklen_t *addrlen, Status &error) {
error.Clear();
#if defined(ANDROID_USE_ACCEPT_WORKAROUND)
// Hack:
@@ -485,7 +471,7 @@ NativeSocket Socket::AcceptSocket(NativeSocket sockfd, struct sockaddr *addr,
// available in older kernels. Using an older libc would fix this issue, but
// introduce other ones, as the old libraries were quite buggy.
int fd = syscall(__NR_accept, sockfd, addr, addrlen);
- if (fd >= 0 && !child_processes_inherit) {
+ if (fd >= 0) {
int flags = ::fcntl(fd, F_GETFD);
if (flags != -1 && ::fcntl(fd, F_SETFD, flags | FD_CLOEXEC) != -1)
return fd;
@@ -494,10 +480,7 @@ NativeSocket Socket::AcceptSocket(NativeSocket sockfd, struct sockaddr *addr,
}
return fd;
#elif defined(SOCK_CLOEXEC) && defined(HAVE_ACCEPT4)
- int flags = 0;
- if (!child_processes_inherit) {
- flags |= SOCK_CLOEXEC;
- }
+ int flags = SOCK_CLOEXEC;
NativeSocket fd = llvm::sys::RetryAfterSignal(
static_cast<NativeSocket>(-1), ::accept4, sockfd, addr, addrlen, flags);
#else
diff --git a/lldb/source/Host/common/TCPSocket.cpp b/lldb/source/Host/common/TCPSocket.cpp
index 2d16b605af9497..5d863954ee8868 100644
--- a/lldb/source/Host/common/TCPSocket.cpp
+++ b/lldb/source/Host/common/TCPSocket.cpp
@@ -38,18 +38,15 @@ using namespace lldb_private;
static const int kType = SOCK_STREAM;
-TCPSocket::TCPSocket(bool should_close, bool child_processes_inherit)
- : Socket(ProtocolTcp, should_close, child_processes_inherit) {}
+TCPSocket::TCPSocket(bool should_close) : Socket(ProtocolTcp, should_close) {}
TCPSocket::TCPSocket(NativeSocket socket, const TCPSocket &listen_socket)
- : Socket(ProtocolTcp, listen_socket.m_should_close_fd,
- listen_socket.m_child_processes_inherit) {
+ : Socket(ProtocolTcp, listen_socket.m_should_close_fd) {
m_socket = socket;
}
-TCPSocket::TCPSocket(NativeSocket socket, bool should_close,
- bool child_processes_inherit)
- : Socket(ProtocolTcp, should_close, child_processes_inherit) {
+TCPSocket::TCPSocket(NativeSocket socket, bool should_close)
+ : Socket(ProtocolTcp, should_close) {
m_socket = socket;
}
@@ -124,8 +121,7 @@ Status TCPSocket::CreateSocket(int domain) {
error = Close();
if (error.Fail())
return error;
- m_socket = Socket::CreateSocket(domain, kType, IPPROTO_TCP,
- m_child_processes_inherit, error);
+ m_socket = Socket::CreateSocket(domain, kType, IPPROTO_TCP, error);
return error;
}
@@ -183,8 +179,8 @@ Status TCPSocket::Listen(llvm::StringRef name, int backlog) {
std::vector<SocketAddress> addresses = SocketAddress::GetAddressInfo(
host_port->hostname.c_str(), nullptr, AF_UNSPEC, SOCK_STREAM, IPPROTO_TCP);
for (SocketAddress &address : addresses) {
- int fd = Socket::CreateSocket(address.GetFamily(), kType, IPPROTO_TCP,
- m_child_processes_inherit, error);
+ int fd =
+ Socket::CreateSocket(address.GetFamily(), kType, IPPROTO_TCP, error);
if (error.Fail() || fd < 0)
continue;
@@ -241,14 +237,13 @@ TCPSocket::Accept(MainLoopBase &loop,
std::vector<MainLoopBase::ReadHandleUP> handles;
for (auto socket : m_listen_sockets) {
auto fd = socket.first;
- auto io_sp =
- std::make_shared<TCPSocket>(fd, false, this->m_child_processes_inherit);
+ auto io_sp = std::make_shared<TCPSocket>(fd, false);
auto cb = [this, fd, sock_cb](MainLoopBase &loop) {
lldb_private::SocketAddress AcceptAddr;
socklen_t sa_len = AcceptAddr.GetMaxLength();
Status error;
- NativeSocket sock = AcceptSocket(fd, &AcceptAddr.sockaddr(), &sa_len,
- m_child_processes_inherit, error);
+ NativeSocket sock =
+ AcceptSocket(fd, &AcceptAddr.sockaddr(), &sa_len, error);
Log *log = GetLog(LLDBLog::Host);
if (error.Fail()) {
LLDB_LOG(log, "AcceptSocket({0}): {1}", fd, error);
diff --git a/lldb/source/Host/common/UDPSocket.cpp b/lldb/source/Host/common/UDPSocket.cpp
index 05d7b2e6506027..f394bda346e1d7 100644
--- a/lldb/source/Host/common/UDPSocket.cpp
+++ b/lldb/source/Host/common/UDPSocket.cpp
@@ -27,12 +27,12 @@ static const int kType = SOCK_DGRAM;
static const char *g_not_supported_error = "Not supported";
-UDPSocket::UDPSocket(NativeSocket socket) : Socket(ProtocolUdp, true, true) {
+UDPSocket::UDPSocket(NativeSocket socket)
+ : Socket(ProtocolUdp, /*should_close=*/true) {
m_socket = socket;
}
-UDPSocket::UDPSocket(bool should_close, bool child_processes_inherit)
- : Socket(ProtocolUdp, should_close, child_processes_inherit) {}
+UDPSocket::UDPSocket(bool should_close) : Socket(ProtocolUdp, should_close) {}
size_t UDPSocket::Send(const void *buf, const size_t num_bytes) {
return ::sendto(m_socket, static_cast<const char *>(buf), num_bytes, 0,
@@ -48,7 +48,7 @@ Status UDPSocket::Listen(llvm::StringRef name, int backlog) {
}
llvm::Expected<std::unique_ptr<UDPSocket>>
-UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit) {
+UDPSocket::CreateConnected(llvm::StringRef name) {
std::unique_ptr<UDPSocket> socket;
Log *log = GetLog(LLDBLog::Connection);
@@ -84,9 +84,9 @@ UDPSocket::Connect(llvm::StringRef name, bool child_processes_inherit) {
for (struct addrinfo *service_info_ptr = service_info_list;
service_info_ptr != nullptr;
service_info_ptr = service_info_ptr->ai_next) {
- auto send_fd = CreateSocket(
- service_info_ptr->ai_family, service_info_ptr->ai_socktype,
- service_info_ptr->ai_protocol, child_processes_inherit, error);
+ auto send_fd =
+ CreateSocket(service_info_ptr->ai_family, service_info_ptr->ai_socktype,
+ service_info_ptr->ai_protocol, error);
if (error.Success()) {
socket.reset(new UDPSocket(send_fd));
socket->m_sockaddr = service_info_ptr;
diff --git a/lldb/source/Host/linux/AbstractSocket.cpp b/lldb/source/Host/linux/AbstractSocket.cpp
index 833234028c865b..8393a80e86e723 100644
--- a/lldb/source/Host/linux/AbstractSocket.cpp
+++ b/lldb/source/Host/linux/AbstractSocket.cpp
@@ -13,8 +13,7 @@
using namespace lldb;
using namespace lldb_private;
-AbstractSocket::AbstractSocket(bool child_processes_inherit)
- : DomainSocket(ProtocolUnixAbstract, child_processes_inherit) {}
+AbstractSocket::AbstractSocket() : DomainSocket(ProtocolUnixAbstract) {}
size_t AbstractSocket::GetNameOffset() const { return 1; }
diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 8a03e47ef3d900..719434182616af 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -535,7 +535,7 @@ lldb::ConnectionStatus ConnectionFileDescriptor::AcceptSocket(
Status *error_ptr) {
Status error;
std::unique_ptr<Socket> listening_socket =
- Socket::Create(socket_protocol, /*child_processes_inherit=*/false, error);
+ Socket::Create(socket_protocol, error);
Socket *accepted_socket;
if (!error.Fail())
@@ -562,8 +562,7 @@ ConnectionFileDescriptor::ConnectSocket(Socket::SocketProtocol socket_protocol,
llvm::StringRef socket_name,
Status *error_ptr) {
Status error;
- std::unique_ptr<Socket> socket =
- Socket::Create(socket_protocol, /*child_processes_inherit=*/false, error);
+ std::unique_ptr<Socket> socket = Socket::Create(socket_protocol, error);
if (!error.Fail())
error = socket->Connect(socket_name);
@@ -644,8 +643,7 @@ ConnectionFileDescriptor::ConnectUDP(llvm::StringRef s,
Status *error_ptr) {
if (error_ptr)
*error_ptr = Status();
- llvm::Expected<std::unique_ptr<UDPSocket>> socket =
- Socket::UdpConnect(s, /*child_processes_inherit=*/false);
+ llvm::Expected<std::unique_ptr<UDPSocket>> socket = Socket::UdpConnect(s);
if (!socket) {
if (error_ptr)
*error_ptr = Status::FromError(socket.takeError());
@@ -690,7 +688,7 @@ ConnectionFileDescriptor::ConnectFD(llvm::StringRef s,
// this. For now, we assume we must assume we don't own it.
std::unique_ptr<TCPSocket> tcp_socket;
- tcp_socket = std::make_unique<TCPSocket>(fd, false, false);
+ tcp_socket = std::make_unique<TCPSocket>(fd, /*should_close=*/false);
// Try and get a socket option from this file descriptor to see if
// this is a socket and set m_is_socket accordingly.
int resuse;
diff --git a/lldb/source/Host/posix/DomainSocket.cpp b/lldb/source/Host/posix/DomainSocket.cpp
index 369123f2239302..0451834630d33f 100644
--- a/lldb/source/Host/posix/DomainSocket.cpp
+++ b/lldb/source/Host/posix/DomainSocket.cpp
@@ -58,24 +58,20 @@ static bool SetSockAddr(llvm::StringRef name, const size_t name_offset,
return true;
}
-DomainSocket::DomainSocket(bool should_close, bool child_processes_inherit)
- : DomainSocket(kInvalidSocketValue, should_close, child_processes_inherit) {
-}
+DomainSocket::DomainSocket(bool should_close)
+ : DomainSocket(kInvalidSocketValue, should_close) {}
-DomainSocket::DomainSocket(NativeSocket socket, bool should_close,
- bool child_processes_inherit)
- : Socket(ProtocolUnixDomain, should_close, child_processes_inherit) {
+DomainSocket::DomainSocket(NativeSocket socket, bool should_close)
+ : Socket(ProtocolUnixDomain, should_close) {
m_socket = socket;
}
-DomainSocket::DomainSocket(SocketProtocol protocol,
- bool child_processes_inherit)
- : Socket(protocol, true, child_processes_inherit) {}
+DomainSocket::DomainSocket(SocketProtocol protocol)
+ : Socket(protocol, /*should_close=*/true) {}
DomainSocket::DomainSocket(NativeSocket socket,
const DomainSocket &listen_socket)
- : Socket(ProtocolUnixDomain, listen_socket.m_sho...
[truncated]
|
ashgti
approved these changes
Nov 26, 2024
It's never set to true. Also, using inheritable FDs in a multithreaded process pretty much guarantees descriptor leaks. It's better to explicitly pass a specific FD to a specific subprocess, which we already mostly can do using the ProcessLaunchInfo FileActions.
labath
force-pushed
the
inherit-socket
branch
from
927b60e to
6471ca1
Compare
labath
added a commit
that referenced
this pull request
Nov 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's never set to true. Also, using inheritable FDs in a multithreaded process pretty much guarantees descriptor leaks. It's better to explicitly pass a specific FD to a specific subprocess, which we already mostly can do using the ProcessLaunchInfo FileActions.