zturner created this revision.
zturner added reviewers: clayborg, granata.enrico.
zturner added a subscriber: lldb-commits.
When we acquire the GIL with `Locker`, we save off the current stdio handles,
redirect them, and when we release the GIL we put the old handles back. This
triggers a potential write-after-close, as follows:
In `ScriptInterpreterPython::ExecuteOneLine`, we set up a pipe, lock the gil,
kick off a thread to do some work, then close the write end of the pipe.
However, the write end of the pipe in this case is the same thing that we set
Python's stdout and stderr handles to, and we haven't yet restored them until
`ExecuteOneLine` ends, due to the scope in which the RAII object is declared.
This has always been fine, but only as a coincidence. In Python 3 it seems
that when you set `stderr` to a new file, it writes to the *original* file
first (probably to flush it). In the case of `ExecuteOneLine`, that file
descriptor has been intentionally closed by caller as a means to break the pipe
and join the other thread it was communicating with, so we get an error.
The fix here is simple: Just acquire the GIL in a tighter scope so that stdin,
stdout, and stderr are reset to their original values before trying to join the
read thread.
It doesn't *seem* like we need to hold the GIL any longer than this since no
Python calls are happening outside of the new smaller scope I've introduced,
but let me know if you can think of anything wrong with this.
http://reviews.llvm.org/D13836
Files:
source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
Index: source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
===================================================================
--- source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
+++ source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.h
@@ -570,9 +570,9 @@
bool
GetEmbeddedInterpreterModuleObjects ();
- PythonObject m_saved_stdin;
- PythonObject m_saved_stdout;
- PythonObject m_saved_stderr;
+ PythonFile m_saved_stdin;
+ PythonFile m_saved_stdout;
+ PythonFile m_saved_stderr;
PythonObject m_main_module;
PythonObject m_lldb_module;
PythonDictionary m_session_dict;
Index: source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
===================================================================
--- source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
+++ source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
@@ -612,7 +612,7 @@
// Flush the file before giving it to python to avoid interleaved output.
in_file.Flush();
- m_saved_stdin = sys_module_dict.GetItemForKey(PythonString("stdin"));
+ m_saved_stdin = sys_module_dict.GetItemForKey(PythonString("stdin")).AsType<PythonFile>();
// This call can deadlock your process if the file is locked
PythonFile new_file(in_file, "r");
sys_module_dict.SetItemForKey (PythonString("stdin"), new_file);
@@ -626,7 +626,7 @@
// Flush the file before giving it to python to avoid interleaved output.
out_file.Flush();
- m_saved_stdout = sys_module_dict.GetItemForKey(PythonString("stdout"));
+ m_saved_stdout = sys_module_dict.GetItemForKey(PythonString("stdout")).AsType<PythonFile>();
PythonFile new_file(out_file, "w");
sys_module_dict.SetItemForKey (PythonString("stdout"), new_file);
@@ -641,7 +641,7 @@
// Flush the file before giving it to python to avoid interleaved output.
err_file.Flush();
- m_saved_stderr = sys_module_dict.GetItemForKey(PythonString("stderr"));
+ m_saved_stderr = sys_module_dict.GetItemForKey(PythonString("stderr")).AsType<PythonFile>();
PythonFile new_file(err_file, "w");
sys_module_dict.SetItemForKey (PythonString("stderr"), new_file);
@@ -812,48 +812,49 @@
FILE *in_file = input_file_sp->GetFile().GetStream();
FILE *out_file = output_file_sp->GetFile().GetStream();
FILE *err_file = error_file_sp->GetFile().GetStream();
- Locker locker(this,
- ScriptInterpreterPython::Locker::AcquireLock |
- ScriptInterpreterPython::Locker::InitSession |
- (options.GetSetLLDBGlobals() ? ScriptInterpreterPython::Locker::InitGlobals : 0) |
- ((result && result->GetInteractive()) ? 0: Locker::NoSTDIN),
- ScriptInterpreterPython::Locker::FreeAcquiredLock |
- ScriptInterpreterPython::Locker::TearDownSession,
- in_file,
- out_file,
- err_file);
-
bool success = false;
-
- // Find the correct script interpreter dictionary in the main module.
- PythonDictionary &session_dict = GetSessionDictionary ();
- if (session_dict.IsValid())
{
- if (GetEmbeddedInterpreterModuleObjects ())
+ Locker locker(this,
+ ScriptInterpreterPython::Locker::AcquireLock |
+ ScriptInterpreterPython::Locker::InitSession |
+ (options.GetSetLLDBGlobals() ? ScriptInterpreterPython::Locker::InitGlobals : 0) |
+ ((result && result->GetInteractive()) ? 0: Locker::NoSTDIN),
+ ScriptInterpreterPython::Locker::FreeAcquiredLock |
+ ScriptInterpreterPython::Locker::TearDownSession,
+ in_file,
+ out_file,
+ err_file);
+
+ // Find the correct script interpreter dictionary in the main module.
+ PythonDictionary &session_dict = GetSessionDictionary ();
+ if (session_dict.IsValid())
{
- if (PyCallable_Check(m_run_one_line_function.get()))
+ if (GetEmbeddedInterpreterModuleObjects ())
{
- PythonObject pargs(PyRefType::Owned, Py_BuildValue("(Os)", session_dict.get(), command));
- if (pargs.IsValid())
+ if (PyCallable_Check(m_run_one_line_function.get()))
{
- PythonObject return_value(PyRefType::Owned,
- PyObject_CallObject(m_run_one_line_function.get(), pargs.get()));
- if (return_value.IsValid())
- success = true;
- else if (options.GetMaskoutErrors() && PyErr_Occurred ())
+ PythonObject pargs(PyRefType::Owned, Py_BuildValue("(Os)", session_dict.get(), command));
+ if (pargs.IsValid())
{
- PyErr_Print();
- PyErr_Clear();
+ PythonObject return_value(PyRefType::Owned,
+ PyObject_CallObject(m_run_one_line_function.get(), pargs.get()));
+ if (return_value.IsValid())
+ success = true;
+ else if (options.GetMaskoutErrors() && PyErr_Occurred ())
+ {
+ PyErr_Print();
+ PyErr_Clear();
+ }
}
}
}
}
- }
- // Flush our output and error file handles
- ::fflush (out_file);
- if (out_file != err_file)
- ::fflush (err_file);
+ // Flush our output and error file handles
+ ::fflush (out_file);
+ if (out_file != err_file)
+ ::fflush (err_file);
+ }
if (join_read_thread)
{
Index: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
===================================================================
--- source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -334,6 +334,7 @@
class PythonFile : public PythonObject
{
public:
+ PythonFile();
PythonFile(File &file, const char *mode);
PythonFile(const char *path, const char *mode);
PythonFile(PyRefType type, PyObject *o);
Index: source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
===================================================================
--- source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -605,6 +605,11 @@
return result;
}
+PythonFile::PythonFile()
+ : PythonObject()
+{
+}
+
PythonFile::PythonFile(File &file, const char *mode)
{
Reset(file, mode);
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
