[Lldb-commits] [PATCH] D69532: [LLDB][PythonFile] fix dangerous borrow semantics on python2

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna created this revision.
lawrence_danna added reviewers: labath, mgorny.
Herald added a project: LLDB.

It is, I think, inherently unsafe to hand a borrowed native file 
object to a python program on python 2.   This is because the python 
file object must be created with a FILE*, not a descriptor.     Even 
holding on to such a reference past its legal scope can result in
use-after-free violations.   Pytyhon programs are not prepared to deal
with that kind of requirement.

Python 3 does not suffer from this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69532

Files:
  lldb/include/lldb/Host/File.h
  lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
  lldb/source/Host/common/File.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp


Index: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
===================================================================
--- lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -1502,19 +1502,28 @@
   file_obj = PyFile_FromFd(file.GetDescriptor(), nullptr, mode, -1, nullptr,
                            "ignore", nullptr, 0);
 #else
-  // We pass ::flush instead of ::fclose here so we borrow the FILE* --
-  // the lldb_private::File still owns it.   NetBSD does not allow
-  // input files to be flushed, so we have to check for that case too.
-  int (*closer)(FILE *);
-  auto opts = file.GetOptions();
+  // It's more or less safe to let a python program borrow a file descriptor.
+  // If they let it dangle and then use it, they'll probably get an exception.
+  // The worst that can happen is they'll wind up doing IO on the wrong
+  // descriptor.   But it would be very unsafe to let a python program borrow
+  // a FILE*.   They could actually crash the program just by keeping a
+  // reference to it around.    Since in python 2 we must have a FILE* and
+  // not a descriptor, we dup the descriptor and fdopen a new FILE* to it
+  // so python can have something it can own safely.
+  auto opts = File::GetOptionsFromMode(mode);
   if (!opts)
     return opts.takeError();
-  if (opts.get() & File::eOpenOptionWrite)
-    closer = ::fflush;
-  else
-    closer = [](FILE *) { return 0; };
-  file_obj = PyFile_FromFile(file.GetStream(), py2_const_cast(""),
-                             py2_const_cast(mode), closer);
+  int fd = file.GetDescriptor();
+  if (!File::DescriptorIsValid(fd))
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "File has no file descriptor");
+  NativeFile dupfile(fd, opts.get(), false);
+  FILE *stream = NativeFile::ReleaseFILE(std::move(dupfile));
+  if (!stream)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "could not create FILE* stream");
+  file_obj = PyFile_FromFile(stream, py2_const_cast(""), py2_const_cast(mode),
+                             ::fclose);
 #endif
 
   if (!file_obj)
Index: lldb/source/Host/common/File.cpp
===================================================================
--- lldb/source/Host/common/File.cpp
+++ lldb/source/Host/common/File.cpp
@@ -304,6 +304,18 @@
   return m_stream;
 }
 
+FILE *NativeFile::ReleaseFILE(NativeFile &&file) {
+  FILE *stream = nullptr;
+  file.GetStream();
+  if (file.m_own_stream) {
+    stream = file.m_stream;
+    file.m_own_stream = false;
+    file.m_stream = nullptr;
+  }
+  file.Close();
+  return stream;
+}
+
 Status NativeFile::Close() {
   Status error;
   if (StreamIsValid()) {
Index: lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
===================================================================
--- lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
+++ lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py
@@ -851,10 +851,6 @@
                     yield sbf
                     sbf.Write(str(i).encode('ascii') + b"\n")
             files = list(i(sbf))
-            # delete them in reverse order, again because each is a borrow
-            # of the previous.
-            while files:
-                files.pop()
         with open(self.out_filename, 'r') as f:
             self.assertEqual(list(range(10)), list(map(int, f.read().strip().split())))
 
Index: lldb/include/lldb/Host/File.h
===================================================================
--- lldb/include/lldb/Host/File.h
+++ lldb/include/lldb/Host/File.h
@@ -411,6 +411,8 @@
   size_t PrintfVarArg(const char *format, va_list args) override;
   llvm::Expected<OpenOptions> GetOptions() const override;
 
+  static FILE *ReleaseFILE(NativeFile &&file);
+
   static char ID;
   virtual bool isA(const void *classID) const override {
     return classID == &ID || File::isA(classID);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D69532.226748.patch
Type: text/x-patch
Size: 4091 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20191028/133ead4d/attachment.bin>