Page MenuHomePhabricator

Fix write-after-close of file descriptor in ScriptInterpreterPython
ClosedPublic

Authored by zturner on Oct 16 2015, 4:43 PM.

Details

Summary

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.

Diff Detail

Event Timeline

zturner updated this revision to Diff 37655.Oct 16 2015, 4:43 PM
zturner retitled this revision from to Fix write-after-close of file descriptor in ScriptInterpreterPython.
zturner updated this object.
zturner added reviewers: clayborg, granata.enrico.
zturner added a subscriber: lldb-commits.

Greg, Enrico, anyone have a chance to take a look at this? I think it's probably just a rubber stamp but rather err on the side of caution.

clayborg accepted this revision.Oct 20 2015, 10:23 AM
clayborg edited edge metadata.

Looks good to me. Wait for Enrico to OK as well.

This revision is now accepted and ready to land.Oct 20 2015, 10:23 AM
granata.enrico accepted this revision.Oct 20 2015, 10:33 AM
granata.enrico edited edge metadata.

Looks reasonable

zturner closed this revision.Oct 26 2015, 1:22 PM