Tcl Source Code

Running the Windows application verifier on trunk shows a use after free on a Windows event handle used for exec pipes. The offending line is here.

The SetEvent generally returns an invalid handle error in this case and we happily ignore the error so it is undetected and life goes on. However, if by chance (however small) the handle value were to be reused by Windows for a different handle, unexpected behaviour would occur as it would not be treated as an invalid handle.

This should be afforded the same level of importance as a user-after-free detected by valgrind.

Proposed fix is here.

Looking more closely at the code, I did not see why that event handle was needed at all. In summary,

• the handle in question is TclPipeThreadInfo.evWakeup. This is initialized from the readable or writable event handles from the channel structure.

• the worker thread however signal read or write completions directly using the readable/writable handles from the channel structure, not evWakeup.

• the only time TclPipeThreadInfo.evWakeup is signalled is on worker thread exit which is when it has already been deallocated by the main thread causing the use-after-free error!

The proposed fix simply expunges all references to the TclPipeThreadInfo.evWakeup. The test suite passes and Application Verifier no longer complains. But someone should review before merge.

Looking at the source, the event handle seems to have been already closed here.

It seems to me that the immediately preceding TclPipeThreadStop wakes up the worker thread, transitioning it to the STOP state where it invokes SetEvent(). In the meanwhile, the code after TclPipeThreadStop immediately closes that event handle before the worker actually runs. Hence the invalid handle error.

I don't know what the synchronization protocol is between the main and worker threads so leaving investigation and fix to those who do.

As an aside, seems like this extends way back to 8.6.