File::SetDescriptor() should require options

lvm_private::File::GetStream() can fail if m_options == 0

It's not clear from the header a File created with a descriptor will be
not be usable by many parts of LLDB unless SetOptions is also called,
but it is.

This is because those parts of LLDB rely on GetStream() to use the
file, and that in turn relies on calling fdopen on the descriptor. When
calling fdopen, GetStream relies on m_options to determine the access
mode. If m_options has never been set, GetStream() will fail.

This patch adds options as a required argument to File::SetDescriptor
and the corresponding constructor.

Patch by: Lawrence D'Anna

Differential revision: https://reviews.llvm.org/D67792

llvm-svn: 372652

11 files changed, 54 insertions, 27 deletions

diff --git a/lldb/include/lldb/Host/File.h b/lldb/include/lldb/Host/File.h
index 688db17e07e..be6c93161ba 100644
--- a/lldb/include/lldb/Host/File.h
+++ b/lldb/include/lldb/Host/File.h
@@ -64,9 +64,9 @@ public:
         m_is_real_terminal(eLazyBoolCalculate),
         m_supports_colors(eLazyBoolCalculate) {}
 
-  File(int fd, bool transfer_ownership)
+  File(int fd, uint32_t options, bool transfer_ownership)
       : IOObject(eFDTypeFile, transfer_ownership), m_descriptor(fd),
-        m_stream(kInvalidStream), m_options(0), m_own_stream(false),
+        m_stream(kInvalidStream), m_options(options), m_own_stream(false),
         m_is_interactive(eLazyBoolCalculate),
         m_is_real_terminal(eLazyBoolCalculate) {}
 
@@ -125,7 +125,7 @@ public:
 
   WaitableHandle GetWaitableHandle() override;
 
-  void SetDescriptor(int fd, bool transfer_ownership);
+  void SetDescriptor(int fd, uint32_t options, bool transfer_ownership);
 
   FILE *GetStream();
 
@@ -332,8 +332,6 @@ public:
 
   size_t PrintfVarArg(const char *format, va_list args);
 
-  void SetOptions(uint32_t options) { m_options = options; }
-
   static bool DescriptorIsValid(int descriptor) { return descriptor >= 0; };
 
 protected:
diff --git a/lldb/source/Core/StreamFile.cpp b/lldb/source/Core/StreamFile.cpp
index ce6e1ea2c18..17662a43604 100644
--- a/lldb/source/Core/StreamFile.cpp
+++ b/lldb/source/Core/StreamFile.cpp
@@ -21,7 +21,7 @@ StreamFile::StreamFile(uint32_t flags, uint32_t addr_size, ByteOrder byte_order)
     : Stream(flags, addr_size, byte_order), m_file() {}
 
 StreamFile::StreamFile(int fd, bool transfer_ownership)
-    : Stream(), m_file(fd, transfer_ownership) {}
+    : Stream(), m_file(fd, File::eOpenOptionWrite, transfer_ownership) {}
 
 StreamFile::StreamFile(FILE *fh, bool transfer_ownership)
     : Stream(), m_file(fh, transfer_ownership) {}
diff --git a/lldb/source/Host/common/File.cpp b/lldb/source/Host/common/File.cpp
index 78b384b66cd..824f088430e 100644
--- a/lldb/source/Host/common/File.cpp
+++ b/lldb/source/Host/common/File.cpp
@@ -93,11 +93,12 @@ int File::GetDescriptor() const {
 
 IOObject::WaitableHandle File::GetWaitableHandle() { return GetDescriptor(); }
 
-void File::SetDescriptor(int fd, bool transfer_ownership) {
+void File::SetDescriptor(int fd, uint32_t options, bool transfer_ownership) {
   if (IsValid())
     Close();
   m_descriptor = fd;
   m_should_close_fd = transfer_ownership;
+  m_options = options;
 }
 
 FILE *File::GetStream() {
diff --git a/lldb/source/Host/common/FileSystem.cpp b/lldb/source/Host/common/FileSystem.cpp
index 0d9417120fd..23b3ebb0b4f 100644
--- a/lldb/source/Host/common/FileSystem.cpp
+++ b/lldb/source/Host/common/FileSystem.cpp
@@ -436,11 +436,10 @@ Status FileSystem::Open(File &File, const FileSpec &file_spec, uint32_t options,
 
   Status error;
   if (!File::DescriptorIsValid(descriptor)) {
-    File.SetDescriptor(descriptor, false);
+    File.SetDescriptor(-1, options, false);
     error.SetErrorToErrno();
   } else {
-    File.SetDescriptor(descriptor, should_close_fd);
-    File.SetOptions(options);
+    File.SetDescriptor(descriptor, options, should_close_fd);
   }
   return error;
 }
diff --git a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
index 9b35640ad8b..f9759ec3caf 100644
--- a/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
+++ b/lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp
@@ -86,8 +86,8 @@ ConnectionFileDescriptor::ConnectionFileDescriptor(bool child_processes_inherit)
 ConnectionFileDescriptor::ConnectionFileDescriptor(int fd, bool owns_fd)
     : Connection(), m_pipe(), m_mutex(), m_shutting_down(false),
       m_waiting_for_accept(false), m_child_processes_inherit(false) {
-  m_write_sp = std::make_shared<File>(fd, owns_fd);
-  m_read_sp = std::make_shared<File>(fd, false);
+  m_write_sp = std::make_shared<File>(fd, File::eOpenOptionWrite, owns_fd);
+  m_read_sp = std::make_shared<File>(fd, File::eOpenOptionRead, false);
 
   Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION |
                                                   LIBLLDB_LOG_OBJECT));
@@ -218,8 +218,10 @@ ConnectionStatus ConnectionFileDescriptor::Connect(llvm::StringRef path,
             m_read_sp = std::move(tcp_socket);
             m_write_sp = m_read_sp;
           } else {
-            m_read_sp = std::make_shared<File>(fd, false);
-            m_write_sp = std::make_shared<File>(fd, false);
+            m_read_sp =
+                std::make_shared<File>(fd, File::eOpenOptionRead, false);
+            m_write_sp =
+                std::make_shared<File>(fd, File::eOpenOptionWrite, false);
           }
           m_uri = *addr;
           return eConnectionStatusSuccess;
@@ -268,8 +270,8 @@ ConnectionStatus ConnectionFileDescriptor::Connect(llvm::StringRef path,
           ::fcntl(fd, F_SETFL, flags);
         }
       }
-      m_read_sp = std::make_shared<File>(fd, true);
-      m_write_sp = std::make_shared<File>(fd, false);
+      m_read_sp = std::make_shared<File>(fd, File::eOpenOptionRead, true);
+      m_write_sp = std::make_shared<File>(fd, File::eOpenOptionWrite, false);
       return eConnectionStatusSuccess;
     }
 #endif
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 12b3005c259..d48e6726aa6 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -901,7 +901,7 @@ ClangExpressionParser::ParseInternal(DiagnosticManager &diagnostic_manager,
     }
 
     if (temp_fd != -1) {
-      lldb_private::File file(temp_fd, true);
+      lldb_private::File file(temp_fd, File::eOpenOptionWrite, true);
       const size_t expr_text_len = strlen(expr_text);
       size_t bytes_written = expr_text_len;
       if (file.Write(expr_text, bytes_written).Success()) {
diff --git a/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm b/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
index ed18a1cb756..03bb70f5163 100644
--- a/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
+++ b/lldb/source/Plugins/Platform/MacOSX/objcxx/PlatformiOSSimulatorCoreSimulatorSupport.mm
@@ -425,11 +425,16 @@ static Status HandleFileAction(ProcessLaunchInfo &launch_info,
           }
         }
         Status posix_error;
+        int oflag = file_action->GetActionArgument();
         int created_fd =
-            open(file_spec.GetPath().c_str(), file_action->GetActionArgument(),
-                 S_IRUSR | S_IWUSR);
+            open(file_spec.GetPath().c_str(), oflag, S_IRUSR | S_IWUSR);
         if (created_fd >= 0) {
-          file.SetDescriptor(created_fd, true);
+          uint32_t file_options = 0;
+          if ((oflag & O_RDWR) || (oflag & O_RDONLY))
+            file_options |= File::eOpenOptionRead;
+          if ((oflag & O_RDWR) || (oflag & O_RDONLY))
+            file_options |= File::eOpenOptionWrite;
+          file.SetDescriptor(created_fd, file_options, true);
           [options setValue:[NSNumber numberWithInteger:created_fd] forKey:key];
           return error; // Success
         } else {
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
index 4284bc616cf..1fdd089dd5d 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp
@@ -537,7 +537,7 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_Close(
   int err = -1;
   int save_errno = 0;
   if (fd >= 0) {
-    File file(fd, true);
+    File file(fd, 0, true);
     Status error = file.Close();
     err = 0;
     save_errno = error.GetError();
@@ -568,7 +568,7 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_pRead(
       }
 
       std::string buffer(count, 0);
-      File file(fd, false);
+      File file(fd, File::eOpenOptionRead, false);
       Status error = file.Read(static_cast<void *>(&buffer[0]), count, offset);
       const ssize_t bytes_read = error.Success() ? count : -1;
       const int save_errno = error.GetError();
@@ -600,7 +600,7 @@ GDBRemoteCommunicationServerCommon::Handle_vFile_pWrite(
     if (packet.GetChar() == ',') {
       std::string buffer;
       if (packet.GetEscapedBinaryData(buffer)) {
-        File file(fd, false);
+        File file(fd, File::eOpenOptionWrite, false);
         size_t count = buffer.size();
         Status error =
             file.Write(static_cast<const void *>(&buffer[0]), count, offset);
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
index 29dd037efd8..97f8388d771 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -1043,9 +1043,9 @@ bool PythonFile::GetUnderlyingFile(File &file) const {
   file.Close();
   // We don't own the file descriptor returned by this function, make sure the
   // File object knows about that.
-  file.SetDescriptor(PyObject_AsFileDescriptor(m_py_obj), false);
   PythonString py_mode = GetAttributeValue("mode").AsType<PythonString>();
-  file.SetOptions(PythonFile::GetOptionsFromMode(py_mode.GetString()));
+  auto options = PythonFile::GetOptionsFromMode(py_mode.GetString());
+  file.SetDescriptor(PyObject_AsFileDescriptor(m_py_obj), options, false);
   return file.IsValid();
 }
 
diff --git a/lldb/source/Target/Process.cpp b/lldb/source/Target/Process.cpp
index 55570d02b9f..cf0cdc00b61 100644
--- a/lldb/source/Target/Process.cpp
+++ b/lldb/source/Target/Process.cpp
@@ -4299,9 +4299,10 @@ public:
   IOHandlerProcessSTDIO(Process *process, int write_fd)
       : IOHandler(process->GetTarget().GetDebugger(),
                   IOHandler::Type::ProcessIO),
-        m_process(process), m_write_file(write_fd, false) {
+        m_process(process),
+        m_write_file(write_fd, File::eOpenOptionWrite, false) {
     m_pipe.CreateNew(false);
-    m_read_file.SetDescriptor(GetInputFD(), false);
+    m_read_file.SetDescriptor(GetInputFD(), File::eOpenOptionRead, false);
   }
 
   ~IOHandlerProcessSTDIO() override = default;
diff --git a/lldb/unittests/Host/FileTest.cpp b/lldb/unittests/Host/FileTest.cpp
index 4f7d0e2b218..8cf1ebd2d99 100644
--- a/lldb/unittests/Host/FileTest.cpp
+++ b/lldb/unittests/Host/FileTest.cpp
@@ -34,3 +34,24 @@ TEST(File, GetWaitableHandleFileno) {
   File file(stream, true);
   EXPECT_EQ(file.GetWaitableHandle(), fd);
 }
+
+TEST(File, GetStreamFromDescriptor) {
+  const auto *Info = testing::UnitTest::GetInstance()->current_test_info();
+  llvm::SmallString<128> name;
+  int fd;
+  llvm::sys::fs::createTemporaryFile(llvm::Twine(Info->test_case_name()) + "-" +
+                                         Info->name(),
+                                     "test", fd, name);
+
+  llvm::FileRemover remover(name);
+  ASSERT_GE(fd, 0);
+
+  File file(fd, File::eOpenOptionWrite, true);
+  ASSERT_TRUE(file.IsValid());
+
+  FILE *stream = file.GetStream();
+  ASSERT_TRUE(stream != NULL);
+
+  EXPECT_EQ(file.GetDescriptor(), fd);
+  EXPECT_EQ(file.GetWaitableHandle(), fd);
+}