[LLDB][Python] fix another fflush issue on NetBSD

[lawrence_danna]

Summary

Here's another instance where we were calling fflush on an input
stream, which is illegal on NetBSD.

[lawrence_danna]

py2_const_cast shouldn't be static as a free function

[lawrence_danna]

found another one

[lawrence_danna]

and another

[mgorny]

I'm sorry but I won't be able to test it until later today. Visually, looks good though.

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

854

For the record, this doesn't really guarantee specific order of destruction. Generally, in Python code you shouldn't rely on destructors being called at all and close files manually. That's why with is commonly used with files.

[lawrence_danna]

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

854

The point is not to rely on the file being closed at a certain time. They point is that each file added to the list is a borrowed reference to the previous one, and we should not allow those references to become dangling by letting the older ones go to zero-references before the younger ones.

Now that I say it, I wonder if it was a bad idea to expose this kind of C++ object lifetime semantics to python programs.
I put in GetFile mainly so that in the case that a SBFile was a proxy of a python file, you could get the python file back.

It might be better to restrict it to that case, and return None instead of a borrowed file in other cases.

On the other hand, there's value in having a consistent API. Under the current semantics this just works:

print("foo",   file=debugger.GetOutputFile().GetFile())

Overall I think I'd lean towards keeping GetFile the way it is, but I can see the argument for restricting it.

[lawrence_danna]

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

854

On second second thought, there's a better way. We don't need to expose the borrow semantics, and we don't need to restrict GetFile like that. We can do the same thing File.cpp does when you ask it for a FILE* on a borrowed FD -- that is, use dup and create one that python can truly own.

[lawrence_danna]

protect python from being exposed to C++ reference borrowing semantics

[lawrence_danna]

There, I think that fixes the issue with borrow semantics and the NetBSD issues.

[mgorny]

Thanks but I'd personally prefer splitting this into a future commit, in case it introduced unrelated issues.

[labath]

I am not thrilled by all of that duping going around. Having multiple FILE objects means that you have multiple independent file caches too. That can cause different kinds of strange behavior if multiple things start reading/writing to the different FILE objects simultaneously. I think I'd rather just keep the existing borrow semantics. I don't think that should be a problem in practice -- it's just that this test is weird because is testing the extreme cases of this behavior (which is fine). Normally you'll just use one level of wrapping and so the underlying file will be naturally kept around, since lldb will still be holding onto it.

[lawrence_danna]

In D69488#1723481, @labath wrote:

I am not thrilled by all of that duping going around. Having multiple FILE objects means that you have multiple independent file caches too. That can cause different kinds of strange behavior if multiple things start reading/writing to the different FILE objects simultaneously. I think I'd rather just keep the existing borrow semantics. I don't think that should be a problem in practice -- it's just that this test is weird because is testing the extreme cases of this behavior (which is fine). Normally you'll just use one level of wrapping and so the underlying file will be naturally kept around, since lldb will still be holding onto it.

I'm not exactly thrilled with it but I will still argue it is the right thing to do. If you look at the docstring for GetFile, it doesn't mention this problem. It says you can't use a borrowed file if it becomes dangling, but it doesn't say you have to delete your references to it before it can dangle. That's because this problem is surprising enough that neither of us thought of it.

Consider this situation: The python programmer makes an object that keeps a reference to debugger and also to debugger.GetOutputFile().GetFile(). When they're done with the object they destroy the debugger and let their object go out of scope.

In python3, they may get an IOError at this point, as the saved output file tries to flush its buffer to a closed file. In python 2 they are exposed to to a use-after free violation and the program could crash!

It is very difficult to write python programs that deal with these kind of semantics correctly, and nothing about this API suggests that it is as dangerous to use as that. I think the correctness and ergonomics concerns outweigh the performance concerns here. Particularly because this issue only affects python 2. Is it really a bigger deal that we generate some extra dups on an end-of-life scripting language than that we potentially crash on that language?

[mgorny]

I have confirmed that your previous revision fixed the problem in question. The newer one would probably require full test suite run which I can't do right now. As I said, I would prefer those two changes to be committed separately, preferably with a few hours delay so that it would be clear if it causes any breakage on buildbot.

[lawrence_danna]

In D69488#1724178, @mgorny wrote:

I have confirmed that your previous revision fixed the problem in question. The newer one would probably require full test suite run which I can't do right now. As I said, I would prefer those two changes to be committed separately, preferably with a few hours delay so that it would be clear if it causes any breakage on buildbot.

which commit id did you test?

[lawrence_danna]

revert to previous version

[mgorny]

Diff 4, I think. Lemme do a quick test with 5. Hopefully it won't request me to rebuild everything again.

[mgorny]

WFM. Thanks!

[labath]

Ok, let's get this thing fixed. We can continue the discussion on the other patch.

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

class FileHandleTestCase(lldbtest.TestBase):
     def test_back_and_forth(self):
         with open(self.out_filename, 'w') as f:
             # at each step here we're borrowing the file, so we have to keep
             # them all alive until the end.
             sbf = lldb.SBFile.Create(f, borrow=True)
             def i(sbf):
                 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/Host/common/File.cpp

 }
 
 Status NativeFile::Close() {
   Status error;
   if (StreamIsValid()) {
     if (m_own_stream) {
       if (::fclose(m_stream) == EOF)
         error.SetErrorToErrno();
-    } else {
+    } else if (m_options & eOpenOptionWrite) {
       if (::fflush(m_stream) == EOF)
         error.SetErrorToErrno();
     }
   }
   if (DescriptorIsValid() && m_own_descriptor) {
     if (::close(m_descriptor) != 0)
       error.SetErrorToErrno();
   }

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

inline llvm::Error exception(const char *s = nullptr) {
   return llvm::make_error<PythonException>(s);
 }
 
 inline llvm::Error keyError() {
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                  "key not in dict");
 }
 
+#if PY_MAJOR_VERSION < 3
+// The python 2 API declares some arguments as char* that should
+// be const char *, but it doesn't actually modify them.
+inline char *py2_const_cast(const char *s) { return const_cast<char *>(s); }
+#else
+inline const char *py2_const_cast(const char *s) { return s; }
+#endif
+
 enum class PyInitialValue { Invalid, Empty };
 
 template <typename T, typename Enable = void> struct PythonFormat;
 
 template <> struct PythonFormat<unsigned long long> {
   static constexpr char format = 'K';
   static auto get(unsigned long long value) { return value; }
 };
public:
   template <typename T> T AsType() const {
     if (!T::Check(m_py_obj))
       return T();
     return T(PyRefType::Borrowed, m_py_obj);
   }
 
   StructuredData::ObjectSP CreateStructuredObject() const;
 
-protected:
-
-#if PY_MAJOR_VERSION < 3
-  // The python 2 API declares some arguments as char* that should
-  // be const char *, but it doesn't actually modify them.
-  static char *py2_const_cast(const char *s) { return const_cast<char *>(s); }
-#else
-  static const char *py2_const_cast(const char *s) { return s; }
-#endif
-
 public:
   template <typename... T>
   llvm::Expected<PythonObject> CallMethod(const char *name,
                                           const T &... t) const {
     const char format[] = {'(', PythonFormat<T>::format..., ')', 0};
     PyObject *obj =
         PyObject_CallMethod(m_py_obj, py2_const_cast(name),
                             py2_const_cast(format), PythonFormat<T>::get(t)...);

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

if (!mode) {
     mode = m.get();
   }
 
   PyObject *file_obj;
 #if PY_MAJOR_VERSION >= 3
   file_obj = PyFile_FromFd(file.GetDescriptor(), nullptr, mode, -1, nullptr,
                            "ignore", nullptr, 0);
 #else
-  // Read through the Python source, doesn't seem to modify these strings
-  char *cmode = const_cast<char *>(mode);
   // We pass ::flush instead of ::fclose here so we borrow the FILE* --
-  // the lldb_private::File still owns it.
-  file_obj =
-      PyFile_FromFile(file.GetStream(), const_cast<char *>(""), cmode, ::fflush);
+  // 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; };
+  file_obj = PyFile_FromFile(file.GetStream(), py2_const_cast(""),
+                             py2_const_cast(mode), closer);
 #endif
 
   if (!file_obj)
     return exception();
 
   return Take<PythonFile>(file_obj);
 }