[Lldb-commits] [lldb] 3dfb949 - [lldb] Check for and use ptsname_r if available
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Wed Oct 7 06:29:42 PDT 2020
Author: Pavel Labath
Date: 2020-10-07T15:29:29+02:00
New Revision: 3dfb94986170c57d9b3f5f2cba039a2eab5e6f13
URL: https://github.com/llvm/llvm-project/commit/3dfb94986170c57d9b3f5f2cba039a2eab5e6f13
DIFF: https://github.com/llvm/llvm-project/commit/3dfb94986170c57d9b3f5f2cba039a2eab5e6f13.diff
LOG: [lldb] Check for and use ptsname_r if available
ptsname is not thread-safe. ptsname_r is available on most (but not all)
systems -- use it preferentially.
In the patch I also improve the thread-safety of the ptsname fallback
path by wrapping it in a mutex. This should guarantee the safety of a
typical ptsname implementation using a single static buffer, as long as
all callers go through this function.
I also remove the error arguments, as the only way this function can
fail is if the "primary" fd is not valid. This is a programmer error as
this requirement is documented, and all callers ensure that is the case.
Differential Revision: https://reviews.llvm.org/D88728
Added:
Modified:
lldb/cmake/modules/LLDBGenerateConfig.cmake
lldb/include/lldb/Host/Config.h.cmake
lldb/include/lldb/Host/PseudoTerminal.h
lldb/source/Host/common/ProcessLaunchInfo.cpp
lldb/source/Host/common/PseudoTerminal.cpp
lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
Removed:
################################################################################
diff --git a/lldb/cmake/modules/LLDBGenerateConfig.cmake b/lldb/cmake/modules/LLDBGenerateConfig.cmake
index 0d3a7fdb1816..caeb3969002b 100644
--- a/lldb/cmake/modules/LLDBGenerateConfig.cmake
+++ b/lldb/cmake/modules/LLDBGenerateConfig.cmake
@@ -7,6 +7,7 @@ include(CheckLibraryExists)
set(CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE)
check_symbol_exists(ppoll poll.h HAVE_PPOLL)
+check_symbol_exists(ptsname_r stdlib.h HAVE_PTSNAME_R)
set(CMAKE_REQUIRED_DEFINITIONS)
check_symbol_exists(sigaction signal.h HAVE_SIGACTION)
check_cxx_symbol_exists(accept4 "sys/socket.h" HAVE_ACCEPT4)
diff --git a/lldb/include/lldb/Host/Config.h.cmake b/lldb/include/lldb/Host/Config.h.cmake
index 7467f429b628..671d71d1c4e3 100644
--- a/lldb/include/lldb/Host/Config.h.cmake
+++ b/lldb/include/lldb/Host/Config.h.cmake
@@ -20,6 +20,8 @@
#cmakedefine01 HAVE_PPOLL
+#cmakedefine01 HAVE_PTSNAME_R
+
#cmakedefine01 HAVE_SIGACTION
#cmakedefine01 HAVE_PROCESS_VM_READV
diff --git a/lldb/include/lldb/Host/PseudoTerminal.h b/lldb/include/lldb/Host/PseudoTerminal.h
index 8a5a233e7748..c2258f15e551 100644
--- a/lldb/include/lldb/Host/PseudoTerminal.h
+++ b/lldb/include/lldb/Host/PseudoTerminal.h
@@ -105,20 +105,11 @@ class PseudoTerminal {
/// A primary pseudo terminal should already be valid prior to
/// calling this function.
///
- /// \param[out] error_str
- /// An pointer to an error that can describe any errors that
- /// occur. This can be NULL if no error status is desired.
- ///
/// \return
- /// The name of the secondary pseudo terminal as a NULL terminated
- /// C. This string that comes from static memory, so a copy of
- /// the string should be made as subsequent calls can change
- /// this value. NULL is returned if this object doesn't have
- /// a valid primary pseudo terminal opened or if the call to
- /// \c ptsname() fails.
+ /// The name of the secondary pseudo terminal.
///
/// \see PseudoTerminal::OpenFirstAvailablePrimary()
- const char *GetSecondaryName(char *error_str, size_t error_len) const;
+ std::string GetSecondaryName() const;
/// Open the first available pseudo terminal.
///
diff --git a/lldb/source/Host/common/ProcessLaunchInfo.cpp b/lldb/source/Host/common/ProcessLaunchInfo.cpp
index 4bc8cda7a006..a4729a28ce74 100644
--- a/lldb/source/Host/common/ProcessLaunchInfo.cpp
+++ b/lldb/source/Host/common/ProcessLaunchInfo.cpp
@@ -222,7 +222,7 @@ llvm::Error ProcessLaunchInfo::SetUpPtyRedirection() {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"PTY::OpenFirstAvailablePrimary failed");
}
- const FileSpec secondary_file_spec(m_pty->GetSecondaryName(nullptr, 0));
+ const FileSpec secondary_file_spec(m_pty->GetSecondaryName());
// Only use the secondary tty if we don't have anything specified for
// input and don't have an action for stdin
diff --git a/lldb/source/Host/common/PseudoTerminal.cpp b/lldb/source/Host/common/PseudoTerminal.cpp
index 72549f1c88ab..4668b09f4fdb 100644
--- a/lldb/source/Host/common/PseudoTerminal.cpp
+++ b/lldb/source/Host/common/PseudoTerminal.cpp
@@ -8,9 +8,11 @@
#include "lldb/Host/PseudoTerminal.h"
#include "lldb/Host/Config.h"
-
+#include "llvm/Support/Errc.h"
#include "llvm/Support/Errno.h"
-
+#include <cassert>
+#include <limits.h>
+#include <mutex>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
@@ -128,15 +130,8 @@ bool PseudoTerminal::OpenSecondary(int oflag, char *error_str,
CloseSecondaryFileDescriptor();
- // Open the primary side of a pseudo terminal
- const char *secondary_name = GetSecondaryName(error_str, error_len);
-
- if (secondary_name == nullptr)
- return false;
-
- m_secondary_fd =
- llvm::sys::RetryAfterSignal(-1, ::open, secondary_name, oflag);
-
+ std::string name = GetSecondaryName();
+ m_secondary_fd = llvm::sys::RetryAfterSignal(-1, ::open, name.c_str(), oflag);
if (m_secondary_fd < 0) {
if (error_str)
ErrnoToStr(error_str, error_len);
@@ -146,32 +141,21 @@ bool PseudoTerminal::OpenSecondary(int oflag, char *error_str,
return true;
}
-// Get the name of the secondary pseudo terminal. A primary pseudo terminal
-// should already be valid prior to calling this function (see
-// OpenFirstAvailablePrimary()).
-//
-// RETURNS:
-// NULL if no valid primary pseudo terminal or if ptsname() fails.
-// The name of the secondary pseudo terminal as a NULL terminated C string
-// that comes from static memory, so a copy of the string should be
-// made as subsequent calls can change this value.
-const char *PseudoTerminal::GetSecondaryName(char *error_str,
- size_t error_len) const {
- if (error_str)
- error_str[0] = '\0';
-
- if (m_primary_fd < 0) {
- if (error_str)
- ::snprintf(error_str, error_len, "%s",
- "primary file descriptor is invalid");
- return nullptr;
- }
- const char *secondary_name = ::ptsname(m_primary_fd);
-
- if (error_str && secondary_name == nullptr)
- ErrnoToStr(error_str, error_len);
-
- return secondary_name;
+std::string PseudoTerminal::GetSecondaryName() const {
+ assert(m_primary_fd >= 0);
+#if HAVE_PTSNAME_R
+ char buf[PATH_MAX];
+ buf[0] = '\0';
+ int r = ptsname_r(m_primary_fd, buf, sizeof(buf));
+ assert(r == 0);
+ return buf;
+#else
+ static std::mutex mutex;
+ std::lock_guard<std::mutex> guard(mutex);
+ const char *r = ptsname(m_primary_fd);
+ assert(r != nullptr);
+ return r;
+#endif
}
// Fork a child process and have its stdio routed to a pseudo terminal.
diff --git a/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm b/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
index 8f36640a66a5..cfd44f9ae5ce 100644
--- a/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
+++ b/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
@@ -407,25 +407,21 @@ static Status HandleFileAction(ProcessLaunchInfo &launch_info,
const int master_fd = launch_info.GetPTY().GetPrimaryFileDescriptor();
if (master_fd != PseudoTerminal::invalid_fd) {
// Check in case our file action open wants to open the secondary
- const char *secondary_path =
- launch_info.GetPTY().GetSecondaryName(NULL, 0);
- if (secondary_path) {
- FileSpec secondary_spec(secondary_path);
- if (file_spec == secondary_spec) {
- int secondary_fd =
- launch_info.GetPTY().GetSecondaryFileDescriptor();
- if (secondary_fd == PseudoTerminal::invalid_fd)
- secondary_fd =
- launch_info.GetPTY().OpenSecondary(O_RDWR, nullptr, 0);
- if (secondary_fd == PseudoTerminal::invalid_fd) {
- error.SetErrorStringWithFormat(
- "unable to open secondary pty '%s'", secondary_path);
- return error; // Failure
- }
- [options setValue:[NSNumber numberWithInteger:secondary_fd]
- forKey:key];
- return error; // Success
+ FileSpec secondary_spec(launch_info.GetPTY().GetSecondaryName());
+ if (file_spec == secondary_spec) {
+ int secondary_fd =
+ launch_info.GetPTY().GetSecondaryFileDescriptor();
+ if (secondary_fd == PseudoTerminal::invalid_fd)
+ secondary_fd =
+ launch_info.GetPTY().OpenSecondary(O_RDWR, nullptr, 0);
+ if (secondary_fd == PseudoTerminal::invalid_fd) {
+ error.SetErrorStringWithFormat(
+ "unable to open secondary pty '%s'", secondary_path);
+ return error; // Failure
}
+ [options setValue:[NSNumber numberWithInteger:secondary_fd]
+ forKey:key];
+ return error; // Success
}
}
Status posix_error;
diff --git a/lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp b/lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
index f1a424ccbca5..67a18bd8de13 100644
--- a/lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
+++ b/lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
@@ -380,8 +380,7 @@ Status ProcessFreeBSD::DoLaunch(Module *module,
FileSpec stdout_file_spec{};
FileSpec stderr_file_spec{};
- const FileSpec dbg_pts_file_spec{
- launch_info.GetPTY().GetSecondaryName(NULL, 0)};
+ const FileSpec dbg_pts_file_spec{launch_info.GetPTY().GetSecondaryName()};
file_action = launch_info.GetFileActionForFD(STDIN_FILENO);
stdin_file_spec =
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 8dea8b980985..9adf25f00b3e 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -818,7 +818,7 @@ Status ProcessGDBRemote::DoLaunch(lldb_private::Module *exe_module,
// does a lot of output.
if ((!stdin_file_spec || !stdout_file_spec || !stderr_file_spec) &&
pty.OpenFirstAvailablePrimary(O_RDWR | O_NOCTTY, nullptr, 0)) {
- FileSpec secondary_name{pty.GetSecondaryName(nullptr, 0)};
+ FileSpec secondary_name(pty.GetSecondaryName());
if (!stdin_file_spec)
stdin_file_spec = secondary_name;
More information about the lldb-commits
mailing list