From 3071ebf7b38341e89be04aa64c257c4643e0648c Mon Sep 17 00:00:00 2001 From: Lawrence D'Anna Date: Wed, 30 Oct 2019 09:45:13 -0700 Subject: [PATCH] [LLDB][PythonFile] fix dangerous borrow semantics on python2 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. Reviewers: labath, mgorny Reviewed By: labath Subscribers: lldb-commits Tags: #lldb Differential Revision: https://reviews.llvm.org/D69532 --- .../test/python_api/file_handle/TestFileHandle.py | 4 ---- .../ScriptInterpreter/Python/PythonDataObjects.cpp | 28 ++++++++++++---------- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py b/lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py index f7f1ad0..5d025a46 100644 --- a/lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py +++ b/lldb/packages/Python/lldbsuite/test/python_api/file_handle/TestFileHandle.py @@ -851,10 +851,6 @@ class FileHandleTestCase(lldbtest.TestBase): 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()))) diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp index df8bac9..ef5eb7a 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -1500,21 +1500,23 @@ Expected PythonFile::FromFile(File &file, const char *mode) { 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) -- 2.7.4