[lldb] Check for and use ptsname_r if available

[labath]

Summary

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.

[mgorny]

lldb/source/Host/common/PseudoTerminal.cpp

149

I would really feel better with a real error handling here. It shouldn't be hard to use ErrorOr here.

[labath]

lldb/source/Host/common/PseudoTerminal.cpp

149

Yeah, but what are you going to do with that value? Pass it to the caller? The existing callers are ignoring the error return anyway, and I don't want to add error handling everywhere as afaict, this function can't fail unless the user messes up the master state (which is not something I want to support).

[mgorny]

lldb/source/Host/common/PseudoTerminal.cpp

149

I get your point but I've literally wasted days because of missing error handling, so I'd really preferred if we wouldn't make it even worse. Though I guess assert is good enough.

[labath]

lldb/source/Host/common/PseudoTerminal.cpp

149

In some ways it's even better because it will point you straight to the place where the assumption is violated, whereas a propagated logic error can manifest itself much farther away (or not at all). :)

[MaskRay]

lldb/source/Host/common/PseudoTerminal.cpp

149

If ptsname/ptsname_r fails, buf will be uninitialized and trigger a use-of-uninitialized-value error.

149

... in a -DLLVM_ENABLE_ASSERTIONS=off build.

This probably still needs some protection.

[labath]

lldb/source/Host/common/PseudoTerminal.cpp

149

What kind of protection did you have it mind? Initialize the buffer to an empty string?

[MaskRay]

LGTM. Worth mentioning that this will be in POSIX issue 8 https://www.austingroupbugs.net/bug_view_page.php?bug_id=508

lldb/source/Host/common/PseudoTerminal.cpp

149

In the case of a non-zero return value, buf[0] = '\0' is probably sufficient to avoid an uninitialized value.

[labath]

In D88728#2315648, @MaskRay wrote:

LGTM. Worth mentioning that this will be in POSIX issue 8 https://www.austingroupbugs.net/bug_view_page.php?bug_id=508

Oh, cool. I didn't know posix is still being worked on :P

lldb/source/Host/common/PseudoTerminal.cpp

149

sounds good.

lldb/cmake/modules/LLDBGenerateConfig.cmake

 # This file contains all the logic for running configure-time checks
 
 include(CheckSymbolExists)
 include(CheckIncludeFile)
 include(CheckIncludeFiles)
 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)
 
 check_include_file(termios.h HAVE_TERMIOS_H)
 check_include_file("sys/types.h" HAVE_SYS_TYPES_H)
 check_include_files("sys/types.h;sys/event.h" HAVE_SYS_EVENT_H)
 

lldb/include/lldb/Host/Config.h.cmake

 
 
 #cmakedefine01 HAVE_SYS_TYPES_H
 
 #cmakedefine01 HAVE_SYS_EVENT_H
 
 #cmakedefine01 HAVE_PPOLL
 
+#cmakedefine01 HAVE_PTSNAME_R
+
 #cmakedefine01 HAVE_SIGACTION
 
 #cmakedefine01 HAVE_PROCESS_VM_READV
 
 #cmakedefine01 HAVE_NR_PROCESS_VM_READV
 
 #ifndef HAVE_LIBCOMPRESSION
 #cmakedefine HAVE_LIBCOMPRESSION

lldb/include/lldb/Host/PseudoTerminal.h

public:
   /// \see PseudoTerminal::ReleaseSecondaryFileDescriptor()
   int GetSecondaryFileDescriptor() const;
 
   /// Get the name of the secondary pseudo terminal.
   ///
   /// 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.
   ///
   /// Opens the first available pseudo terminal with \a oflag as the
   /// permissions. The opened primary file descriptor is stored in this object
   /// and can be accessed by calling the
   /// PseudoTerminal::GetPrimaryFileDescriptor() accessor. Clients can call the
   /// PseudoTerminal::ReleasePrimaryFileDescriptor() accessor function if they

lldb/source/Host/common/ProcessLaunchInfo.cpp

#if !defined(_WIN32)
   // intended for a system call in generic code.  But this will have to
   // do for now.
   open_flags |= O_CLOEXEC;
 #endif
   if (!m_pty->OpenFirstAvailablePrimary(open_flags, nullptr, 0)) {
     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
   if (GetFileActionForFD(STDIN_FILENO) == nullptr)
     AppendOpenFileAction(STDIN_FILENO, secondary_file_spec, true, false);
 
   // Only use the secondary tty if we don't have anything specified for
   // output and don't have an action for stdout

lldb/source/Host/common/PseudoTerminal.cpp

 //===-- PseudoTerminal.cpp ------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #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>
 #if defined(TIOCSCTTY)
 #include <sys/ioctl.h>
 #endif
 
 #include "lldb/Host/PosixApi.h"

 //  True when successful, false indicating an error occurred.
 bool PseudoTerminal::OpenSecondary(int oflag, char *error_str,
                                    size_t error_len) {
   if (error_str)
     error_str[0] = '\0';
 
   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);
     return false;
   }
 
   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.
 //
 // In the parent process when a valid pid is returned, the primary file
 // descriptor can be used as a read/write access to stdio of the child process.
 //
 // In the child process the stdin/stdout/stderr will already be routed to the

lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm

case FileAction::eFileActionDuplicate:
       break;
 
     case FileAction::eFileActionOpen: {
       FileSpec file_spec = file_action->GetFileSpec();
       if (file_spec) {
         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());
         Status posix_error;
         int oflag = file_action->GetActionArgument();
         int created_fd =
             open(file_spec.GetPath().c_str(), oflag, S_IRUSR | S_IWUSR);
         if (created_fd >= 0) {
           auto file_options = File::OpenOptions(0);
           if ((oflag & O_RDWR) || (oflag & O_RDONLY))
             file_options |= File::eOpenOptionRead;

lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp

Status ProcessFreeBSD::DoLaunch(Module *module,
 
   const lldb_private::FileAction *file_action;
 
   // Default of empty will mean to use existing open file descriptors
   FileSpec stdin_file_spec{};
   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 =
       GetFileSpec(file_action, stdin_file_spec, dbg_pts_file_spec);
 
   file_action = launch_info.GetFileActionForFD(STDOUT_FILENO);
   stdout_file_spec =
       GetFileSpec(file_action, stdout_file_spec, dbg_pts_file_spec);

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

if (error.Success()) {
                                    FileSpec::Style::native);
       } else if (platform_sp && platform_sp->IsHost()) {
         // If the debugserver is local and we aren't disabling STDIO, lets use
         // a pseudo terminal to instead of relying on the 'O' packets for stdio
         // since 'O' packets can really slow down debugging if the inferior
         // 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;
 
           if (!stdout_file_spec)
             stdout_file_spec = secondary_name;
 
           if (!stderr_file_spec)