From 2856422eab2169928a44ebbf00e8b8d8ad7e5916 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 7 Apr 2015 22:17:08 +0100 Subject: [PATCH] Fix a dangerous cross-thread memory access. When a winhandl.c input thread returns EOF to the main thread, the latter might immediately delete the input thread's context. I carefully wrote in a comment that in that case we had to not touch ctx ever again after signalling to the main thread - but the test for whether that was true, which also touched ctx, itself came _after_ the SetEvent which sent that signal. Ahem. Spotted by Minefield, which it looks as if I haven't run for a while. (cherry picked from commit 9fec2e773873e28f1409f5e1eefaf03483070050) --- windows/winhandl.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/windows/winhandl.c b/windows/winhandl.c index d81054f3..7c282fc7 100644 --- a/windows/winhandl.c +++ b/windows/winhandl.c @@ -115,7 +115,7 @@ static DWORD WINAPI handle_input_threadfunc(void *param) struct handle_input *ctx = (struct handle_input *) param; OVERLAPPED ovl, *povl; HANDLE oev; - int readret, readlen; + int readret, readlen, finished; if (ctx->flags & HANDLE_FLAG_OVERLAPPED) { povl = &ovl; @@ -165,18 +165,20 @@ static DWORD WINAPI handle_input_threadfunc(void *param) (ctx->flags & HANDLE_FLAG_IGNOREEOF)) continue; + /* + * If we just set ctx->len to 0, that means the read operation + * has returned end-of-file. Telling that to the main thread + * will cause it to set its 'defunct' flag and dispose of the + * handle structure at the next opportunity, in which case we + * mustn't touch ctx at all after the SetEvent. (Hence we do + * even _this_ check before the SetEvent.) + */ + finished = (ctx->len == 0); + SetEvent(ctx->ev_to_main); - if (!ctx->len) { - /* - * The read operation has returned end-of-file. Telling - * that to the main thread will cause it to set its - * 'defunct' flag and dispose of the handle structure at - * the next opportunity, so we must not touch ctx at all - * after this. - */ + if (finished) break; - } WaitForSingleObject(ctx->ev_from_main, INFINITE); if (ctx->done) { -- 2.45.2