From 21c2e451dade78bf5590af4b84a68fc608840317 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Wed, 7 Apr 2021 20:15:34 +0100 Subject: [PATCH] winpgnt: fix crash if deferred-decryption passphrase is wrong. Thanks to Jacob for spotting this one: when we hand a passphrase back to pageant.c via pageant_passphrase_request_success(), if the key doesn't decrypt successfully, pageant.c responds by immediately issuing another passphrase prompt - and it does it _synchronously_, by calling back from within pageant_passphrase_request_success(). In this case, the effect is that we end up in ask_passphrase_common(), which starts by asserting that nonmodal_passphrase_hwnd is NULL - but it wasn't NULL _quite_ yet, because end_passphrase_dialog() was expecting to clean it up immediately after pageant_passphrase_request_success() returned, i.e. just too late. The heavyweight fix would be to arrange a toplevel callback to defer opening the new window until after the old one had been cleaned up. But in this case I don't think there's any need: it's enough to simply do the operations in end_passphrase_dialog() in the opposite order, so that first we destroy the old window and set nonmodal_passphrase_hwnd back to NULL, and _then_ we call into pageant.c which might call us back and open a fresh window. --- windows/winpgnt.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/windows/winpgnt.c b/windows/winpgnt.c index 98066f3b..38e79149 100644 --- a/windows/winpgnt.c +++ b/windows/winpgnt.c @@ -188,6 +188,21 @@ static void end_passphrase_dialog(HWND hwnd, INT_PTR result) if (p->modal) { EndDialog(hwnd, result); } else { + /* + * Destroy this passphrase dialog box before passing the + * results back to pageant.c, to avoid re-entrancy issues. + * + * If we successfully got a passphrase from the user, but it + * was _wrong_, then pageant_passphrase_request_success will + * respond by calling back - synchronously - to our + * ask_passphrase() implementation, which will expect the + * previous value of nonmodal_passphrase_hwnd to have already + * been cleaned up. + */ + SetWindowLongPtr(hwnd, GWLP_USERDATA, (LONG_PTR) NULL); + DestroyWindow(hwnd); + nonmodal_passphrase_hwnd = NULL; + if (result) pageant_passphrase_request_success( p->dlgid, ptrlen_from_asciz(p->passphrase)); @@ -196,9 +211,6 @@ static void end_passphrase_dialog(HWND hwnd, INT_PTR result) burnstr(p->passphrase); sfree(p); - SetWindowLongPtr(hwnd, GWLP_USERDATA, (LONG_PTR) NULL); - DestroyWindow(hwnd); - nonmodal_passphrase_hwnd = NULL; } }