[LLDB][PythonFile] fix dangerous borrow semantics on python2

[lawrence_danna]

Summary

It is inherently unsafe to allow a python program to manipulate borrowed
memory from a python object's destructor. It would be nice to
flush a borrowed file when python is finished with it, but it's not safe
to do on python 2.

Python 3 does not suffer from this issue.

[labath]

So, if I understand correctly. The problem here is the final call to fflush, which can end up referencing a closed FILE*. Can we just not call fflush then? It shouldn't be really needed -- if everything goes through the same FILE* object, the C library will make sure the writes are available to anyone who tries to read through that object.

I don't buy the argument that holding onto a FD-backed object after the FD has been closed is somehow "safer" than holding onto a FILE*. They both produce undefined behavior, and given how FDs are recycled, it's very likely that this dangling object will end up writing to a random other open file -- that may be even worse than crashing. If we're not happy with that requirement then we can make GetFile() return a real python object, which will hold onto the lldb_private::File instance and handle these use-after-free cases in a reliable manner.

[lawrence_danna]

In D69532#1724806, @labath wrote:

So, if I understand correctly. The problem here is the final call to fflush, which can end up referencing a closed FILE*. Can we just not call fflush then?

Hrm, maybe. I'll try it and see if anything breaks.

It shouldn't be really needed -- if everything goes through the same FILE* object, the C library will make sure the writes are available to anyone who tries to read through that object.
I don't buy the argument that holding onto a FD-backed object after the FD has been closed is somehow "safer" than holding onto a FILE*. They both produce undefined behavior,

But this is a fundamental difference between python and C++. In C++ there's only one level of UB. If you do something illegal, your program can crash, demons come out of your nose, whatever. In python, unless you're using something like ctypes, UB means your program misbehaves, but should never mean that the interpreter crashes. An interpreter crash in python is not the moral equivalent of a segfault in C++, it's the equivalent of a kernel panic. If unprivileged C++ code crashes the kernel, that's a bug in the kernel, whether UB was involved or not. If a non-ctypes python program crashes the interpreter, that's a bug in Python or some loadable module, no matter how incorrect the python program was.

The other aspect here is that it is reasonable to ask a python programmer not to use a closed file. I don't think it is ever safe to ask a python programmer to ensure that reference counts get decremented in a particular order.

[lawrence_danna]

just don't even flush

[lawrence_danna]

fix the test too

[lawrence_danna]

@labath looks like you were right, just not flushing seems to work fine.

[labath]

In D69532#1725772, @lawrence_danna wrote:

It shouldn't be really needed -- if everything goes through the same FILE* object, the C library will make sure the writes are available to anyone who tries to read through that object.
I don't buy the argument that holding onto a FD-backed object after the FD has been closed is somehow "safer" than holding onto a FILE*. They both produce undefined behavior,

But this is a fundamental difference between python and C++. In C++ there's only one level of UB. If you do something illegal, your program can crash, demons come out of your nose, whatever. In python, unless you're using something like ctypes, UB means your program misbehaves, but should never mean that the interpreter crashes. An interpreter crash in python is not the moral equivalent of a segfault in C++, it's the equivalent of a kernel panic. If unprivileged C++ code crashes the kernel, that's a bug in the kernel, whether UB was involved or not. If a non-ctypes python program crashes the interpreter, that's a bug in Python or some loadable module, no matter how incorrect the python program was.

Yes, I get that. What I was trying to say that using a fd-based python file after it has been closed introduces exactly the same level of UB as for a FILE*-based one. I mean, for all you know, the fd might be recycled to point to /dev/sda, and writing to it can make your whole system unbootable.

The difference was that in the FILE* case the mere act of closing constituted a "use", while for a fd file we just happily did nothing. Not fflushing makes this aspect of the two implementations equal. It is true that this introduces some potential difference in behavior as a FILE* involves userspace buffers, whereas for an fd everything would go to the kernel straight away (at least on posix systems). However, cloning a bunch of FILE*s would introduce other kinds of inconsistencies, so I think this is a better tradeoff.

lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py

def test_back_and_forth(self):
                 for i in range(10):
                     f = sbf.GetFile()
                     self.assertEqual(f.mode, "w")
                     yield f
                     sbf = lldb.SBFile.Create(f, borrow=True)
                     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())))
 
 
     @add_test_categories(['pyapi'])
     def test_set_filehandle_none(self):
         self.assertRaises(Exception, self.debugger.SetOutputFile, None)
         self.assertRaises(Exception, self.debugger.SetOutputFile, "ham sandwich")

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

if (!mode) {
     if (!m)
       return m.takeError();
     mode = m.get();
   }
 
   PyObject *file_obj;
 #if PY_MAJOR_VERSION >= 3
   file_obj = PyFile_FromFd(file.GetDescriptor(), nullptr, mode, -1, nullptr,
-                           "ignore", nullptr, 0);
+                           "ignore", nullptr, /*closefd=*/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();
-  if (!opts)
-    return opts.takeError();
-  if (opts.get() & File::eOpenOptionWrite)
-    closer = ::fflush;
-  else
-    closer = [](FILE *) { return 0; };
+  // I'd like to pass ::fflush here if the file is writable,  so that
+  // when the python side destructs the file object it will be flushed.
+  // However, this would be dangerous.    It can cause fflush to be called
+  // after fclose if the python program keeps a reference to the file after
+  // the original lldb_private::File has been destructed.
+  //
+  // It's all well and good to ask a python program not to use a closed file
+  // but asking a python program to make sure objects get released in a
+  // particular order is not safe.
+  //
+  // The tradeoff here is that if a python 2 program wants to make sure this
+  // file gets flushed, they'll have to do it explicitly or wait untill the
+  // original lldb File itself gets flushed.
   file_obj = PyFile_FromFile(file.GetStream(), py2_const_cast(""),
-                             py2_const_cast(mode), closer);
+                             py2_const_cast(mode), [](FILE *) { return 0; });
 #endif
 
   if (!file_obj)
     return exception();
 
   return Take<PythonFile>(file_obj);
 }