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);
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);
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
