File::SetDescriptor() should require options

[lawrence_danna]

Summary

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.

[labath]

It doesn't look like this will work on windows (I find no trace of fcntl in the windows docs). Even if we find a way to guess the open mode on windows somehow, I am not convinced that is the right way forward here. I would rather we find a way to fix the api so that it is harder to use incorrectly, then trying to guess what should happen when the api is used in a potentially incorrect way.

What is original reason you needed this patch for? Could we just return a null FILE* if the user didn't call SetOptions?

[lawrence_danna]

In D67792#1676556, @labath wrote:

It doesn't look like this will work on windows (I find no trace of fcntl in the windows docs). Even if we find a way to guess the open mode on windows somehow, I am not convinced that is the right way forward here. I would rather we find a way to fix the api so that it is harder to use incorrectly, then trying to guess what should happen when the api is used in a potentially incorrect way.

What is original reason you needed this patch for? Could we just return a null FILE* if the user didn't call SetOptions?

While I was writing tests for my python files branch, I first tested the simple case of creating a SBFile with a descriptor, and found to my surprise that almost nothing in LLDB could actually write to that file, because everything expected a FILE* stream and it couldn't make one.

Should I just add an options argument to SetDescriptor instead?

[lawrence_danna]

Added options to SetDescriptor instead of using ::fcntl

[JDevlieghere]

Do we still need SetOptions after this? Are there cases where the value needs to change after construction? If not I would consider removing that function. Otherwise this LGTM if Pavel doesn't have any other concerns.

[lawrence_danna]

removed SetOptions()

[labath]

This looks much better to me, but maybe you could add some unit tests for the use cases this fixes (I guess that's the use case of creating a File object with an fd, and the writing(?) to it).

[lawrence_danna]

added unit test

lldb/include/lldb/Host/File.h

public:
 
   File(FILE *fh, bool transfer_ownership)
       : IOObject(eFDTypeFile, false), m_descriptor(kInvalidDescriptor),
         m_stream(fh), m_options(0), m_own_stream(transfer_ownership),
         m_is_interactive(eLazyBoolCalculate),
         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) {}
 
   /// Destructor.
   ///
   /// The destructor is virtual in case this class is subclassed.
   ~File() override;
 
public:
   Status Close() override;
 
   void Clear();
 
   int GetDescriptor() const;
 
   WaitableHandle GetWaitableHandle() override;
 
-  void SetDescriptor(int fd, bool transfer_ownership);
+  void SetDescriptor(int fd, uint32_t options, bool transfer_ownership);
 
   FILE *GetStream();
 
   void SetStream(FILE *fh, bool transfer_ownership);
 
   /// Read bytes from a file from the current file position.
   ///
   /// NOTE: This function is NOT thread safe. Use the read function
public:
   ///
   /// \param[in] ...
   ///     Variable arguments that are needed for the printf style
   ///     format string \a format.
   size_t Printf(const char *format, ...) __attribute__((format(printf, 2, 3)));
 
   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:
   bool DescriptorIsValid() const { return DescriptorIsValid(m_descriptor); }
 
   bool StreamIsValid() const { return m_stream != kInvalidStream; }
 
   void CalculateInteractiveAndTerminal();

lldb/source/Core/StreamFile.cpp

 
 // StreamFile constructor
 StreamFile::StreamFile() : Stream(), m_file() {}
 
 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) {}
 
 StreamFile::StreamFile(const char *path) : Stream(), m_file() {
   FileSystem::Instance().Open(m_file, FileSpec(path),
                               File::eOpenOptionWrite |
                                   File::eOpenOptionCanCreate |

lldb/source/Host/common/File.cpp

#endif
   }
 
   // Invalid descriptor and invalid stream, return invalid descriptor.
   return kInvalidDescriptor;
 }
 
 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() {
   if (!StreamIsValid()) {
     if (DescriptorIsValid()) {
       const char *mode = GetStreamOpenModeFromOptions(m_options);
       if (mode) {
         if (!m_should_close_fd) {

lldb/source/Host/common/FileSystem.cpp

Status FileSystem::Open(File &File, const FileSpec &file_spec, uint32_t options,
   if (!path)
     return Status(path.getError());
 
   int descriptor = llvm::sys::RetryAfterSignal(
       -1, OpenWithFS, *this, path->c_str(), open_flags, open_mode);
 
   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;
 }
 
 ErrorOr<std::string> FileSystem::GetExternalPath(const llvm::Twine &path) {
   if (!m_mapped)
     return path.str();
 

lldb/source/Host/posix/ConnectionFileDescriptorPosix.cpp

Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_CONNECTION |
                                                   LIBLLDB_LOG_OBJECT));
   LLDB_LOGF(log, "%p ConnectionFileDescriptor::ConnectionFileDescriptor ()",
             static_cast<void *>(this));
 }
 
 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));
   LLDB_LOGF(log,
             "%p ConnectionFileDescriptor::ConnectionFileDescriptor (fd = "
             "%i, owns_fd = %i)",
             static_cast<void *>(this), fd, owns_fd);
   OpenCommandPipe();
else if ((addr = GetURLAddress(path, FD_SCHEME))) {
           // this is a socket and set m_is_socket accordingly.
           int resuse;
           bool is_socket =
               !!tcp_socket->GetOption(SOL_SOCKET, SO_REUSEADDR, resuse);
           if (is_socket) {
             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;
         }
       }
 
       if (error_ptr)
         error_ptr->SetErrorStringWithFormat("invalid file descriptor: \"%s\"",
else if ((addr = GetURLAddress(path, FD_SCHEME))) {
 
       int flags = ::fcntl(fd, F_GETFL, 0);
       if (flags >= 0) {
         if ((flags & O_NONBLOCK) == 0) {
           flags |= O_NONBLOCK;
           ::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
     if (error_ptr)
       error_ptr->SetErrorStringWithFormat("unsupported connection URL: '%s'",
                                           path.str().c_str());
     return eConnectionStatusError;
   }

lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

if (FileSpec tmpdir_file_spec = HostInfo::GetProcessTempDir()) {
       tmpdir_file_spec.AppendPathComponent("lldb-%%%%%%.expr");
       std::string temp_source_path = tmpdir_file_spec.GetPath();
       llvm::sys::fs::createUniqueFile(temp_source_path, temp_fd, result_path);
     } else {
       llvm::sys::fs::createTemporaryFile("lldb", "expr", temp_fd, result_path);
     }
 
     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()) {
         if (bytes_written == expr_text_len) {
           file.Close();
           if (auto fileEntry =
                   m_compiler->getFileManager().getFile(result_path)) {
             source_mgr.setMainFileID(source_mgr.createFileID(

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

case FileAction::eFileActionOpen: {
               }
               [options setValue:[NSNumber numberWithInteger:slave_fd]
                          forKey:key];
               return error; // Success
             }
           }
         }
         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 {
           posix_error.SetErrorToErrno();
           error.SetErrorStringWithFormat("unable to open file '%s': %s",
                                          file_spec.GetPath().c_str(),
                                          posix_error.AsCString());
         }

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

 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerCommon::Handle_vFile_Close(
     StringExtractorGDBRemote &packet) {
   packet.SetFilePos(::strlen("vFile:close:"));
   int fd = packet.GetS32(-1);
   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();
   } else {
     save_errno = EINVAL;
   }
   StreamString response;
   response.PutChar('F');
if (packet.GetChar() == ',') {
     if (packet.GetChar() == ',') {
       off_t offset = packet.GetU64(UINT32_MAX);
       if (count == SIZE_MAX) {
         response.Printf("F-1:%i", EINVAL);
         return SendPacketNoLock(response.GetString());
       }
 
       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();
       response.PutChar('F');
       response.Printf("%zi", bytes_read);
       if (save_errno)
         response.Printf(",%i", save_errno);
       else {
GDBRemoteCommunicationServerCommon::Handle_vFile_pWrite(
   response.PutChar('F');
 
   int fd = packet.GetU32(UINT32_MAX);
   if (packet.GetChar() == ',') {
     off_t offset = packet.GetU64(UINT32_MAX);
     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);
         const ssize_t bytes_written = error.Success() ? count : -1;
         const int save_errno = error.GetError();
         response.Printf("%zi", bytes_written);
         if (save_errno)
           response.Printf(",%i", save_errno);

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp

 
 bool PythonFile::GetUnderlyingFile(File &file) const {
   if (!IsValid())
     return false;
 
   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();
 }
 
 #endif

lldb/source/Target/Process.cpp

void Process::STDIOReadThreadBytesReceived(void *baton, const void *src,
   process->AppendSTDOUT(static_cast<const char *>(src), src_len);
 }
 
 class IOHandlerProcessSTDIO : public IOHandler {
 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;
 
   // Each IOHandler gets to run until it is done. It should read data from the
   // "in" and place output into "out" and "err and return when done.
   void Run() override {
     if (!m_read_file.IsValid() || !m_write_file.IsValid() ||