From 152130e003682b163f4057d36264ff45f77c05e9 Mon Sep 17 00:00:00 2001 From: Selva Nair Date: Tue, 24 Jan 2023 08:40:26 -0800 Subject: [PATCH] Check return value of SetProp() (#591) * Check return value of SetProp - If SetProp() is unsuccessful, we'll crash later when GetProp() returns null. Add a check, log the error and close the dialog. We could abort here, but closing the current dialog and possibly the corresponding connection, provides a chance for the user to fix the OOM condition which is the most likely cause of SetProp() failure. - In pkcs11.c if SetProp() fails just do not use bold font for header instead of leaking the font resource. Also correct a bad fixup in commit 80697ecae6: hfontProp was not set! Github: Fixes OpenVPN/openvpn-gui#577 Signed-off-by: Selva Nair --- as.c | 6 +++--- openvpn.c | 21 +++++++++++++-------- openvpn.h | 8 ++++++++ pkcs11.c | 11 +++++++---- proxy.c | 2 +- 5 files changed, 32 insertions(+), 16 deletions(-) diff --git a/as.c b/as.c index 75556dc..6028688 100644 --- a/as.c +++ b/as.c @@ -230,7 +230,7 @@ CRDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) switch (msg) { case WM_INITDIALOG: param = (auth_param_t*)lParam; - SetProp(hwndDlg, cfgProp, (HANDLE)param); + TRY_SETPROP(hwndDlg, cfgProp, (HANDLE)param); WCHAR *wstr = Widen(param->str); if (!wstr) { @@ -280,7 +280,6 @@ CRDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) return TRUE; case WM_NCDESTROY: - param = (auth_param_t*)GetProp(hwndDlg, cfgProp); RemoveProp(hwndDlg, cfgProp); break; } @@ -614,7 +613,7 @@ ImportProfileFromURLDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lPa { case WM_INITDIALOG: type = (server_type_t) lParam; - SetProp(hwndDlg, cfgProp, (HANDLE)lParam); + TRY_SETPROP(hwndDlg, cfgProp, (HANDLE)lParam); SetStatusWinIcon(hwndDlg, ID_ICO_APP); if (type == server_generic) @@ -700,6 +699,7 @@ ImportProfileFromURLDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lPa return TRUE; case WM_NCDESTROY: + RemoveProp(hwndDlg, cfgProp); break; } diff --git a/openvpn.c b/openvpn.c index 630361e..2ee1427 100644 --- a/openvpn.c +++ b/openvpn.c @@ -490,7 +490,7 @@ AutoCloseSetup(HWND hwnd, UINT btn, UINT timeout, UINT txtid, UINT txtres) SetTimer(hwnd, 1, 500, AutoCloseHandler); /* using timer id = 1 */ if (txtid && txtres) SetDlgItemText(hwnd, txtid, LoadLocalizedString(txtres, timeout)); - SetPropW(hwnd, L"AutoClose", (HANDLE) ac); + SetPropW(hwnd, L"AutoClose", (HANDLE) ac); /* failure not critical */ } } @@ -509,7 +509,7 @@ UserAuthDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) case WM_INITDIALOG: /* Set connection for this dialog and show it */ param = (auth_param_t *) lParam; - SetProp(hwndDlg, cfgProp, (HANDLE) param); + TRY_SETPROP(hwndDlg, cfgProp, (HANDLE) param); SetStatusWinIcon(hwndDlg, ID_ICO_APP); if (param->str) @@ -699,7 +699,7 @@ GenericPassDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) { case WM_INITDIALOG: param = (auth_param_t *) lParam; - SetProp(hwndDlg, cfgProp, (HANDLE) param); + TRY_SETPROP(hwndDlg, cfgProp, (HANDLE) param); WCHAR *wstr = Widen (param->str); if (!wstr) @@ -893,7 +893,7 @@ PrivKeyPassDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) case WM_INITDIALOG: /* Set connection for this dialog and show it */ c = (connection_t *) lParam; - SetProp(hwndDlg, cfgProp, (HANDLE) c); + TRY_SETPROP(hwndDlg, cfgProp, (HANDLE) c); AppendTextToCaption (hwndDlg, c->config_name); if (RecallKeyPass(c->config_name, passphrase) && wcslen(passphrase) && c->failed_psw_attempts == 0) @@ -1964,7 +1964,14 @@ StatusDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) SetStatusWinIcon(hwndDlg, ID_ICO_CONNECTING); /* Set connection for this dialog */ - SetProp(hwndDlg, cfgProp, (HANDLE) c); + if (!SetPropW(hwndDlg, cfgProp, (HANDLE) c)) + { + MsgToEventLog(EVENTLOG_ERROR_TYPE, L"%hs:%d SetProp failed (error = 0x%08x)", + __func__, __LINE__, GetLastError()); + DisconnectDaemon(c); + DestroyWindow(hwndDlg); + break; + } /* Create log window */ HWND hLogWnd = CreateWindowEx(0, RICHEDIT_CLASS, NULL, @@ -2058,9 +2065,7 @@ StatusDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) case WM_SHOWWINDOW: if (wParam == TRUE) { - c = (connection_t *) GetProp(hwndDlg, cfgProp); - if (c->hwndStatus) - SetFocus(GetDlgItem(c->hwndStatus, ID_EDT_LOG)); + SetFocus(GetDlgItem(hwndDlg, ID_EDT_LOG)); } return FALSE; diff --git a/openvpn.h b/openvpn.h index 7a6b153..acec295 100644 --- a/openvpn.h +++ b/openvpn.h @@ -25,6 +25,14 @@ #include "options.h" +#define TRY_SETPROP(hwnd, name, p) \ + do { if (SetPropW(hwnd, name, p)) break; \ + MsgToEventLog(EVENTLOG_ERROR_TYPE, L"%hs:%d GetProp returned null", \ + __func__, __LINE__); \ + EndDialog(hwnd, IDABORT); \ + return false; \ + } while(0) + BOOL StartOpenVPN(connection_t *); void StopOpenVPN(connection_t *); void DetachOpenVPN(connection_t *); diff --git a/pkcs11.c b/pkcs11.c index 7a1d2ac..426fda2 100644 --- a/pkcs11.c +++ b/pkcs11.c @@ -37,7 +37,7 @@ #include extern options_t o; -static const wchar_t *hfontProp; +static const wchar_t *hfontProp = L"header_font"; /* state of list array */ #define STATE_GET_COUNT 1 @@ -414,10 +414,13 @@ pkcs11_listview_init(HWND parent) lf.lfWeight = FW_BOLD; HFONT hfb = CreateFontIndirect(&lf); - if (hfb) + if (hfb && SetPropW(parent, hfontProp, (HANDLE)hfb)) { SendMessage(ListView_GetHeader(lv), WM_SETFONT, (WPARAM)hfb, 1); - SetProp(parent, hfontProp, (HANDLE)hfb); + } + else if (hfb) + { + DeleteObject(hfb); } } @@ -569,7 +572,7 @@ QueryPkcs11DialogProc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) { case WM_INITDIALOG: c = (connection_t *) lParam; - SetProp(hwndDlg, cfgProp, (HANDLE)lParam); + TRY_SETPROP(hwndDlg, cfgProp, (HANDLE)lParam); SetStatusWinIcon(hwndDlg, ID_ICO_APP); /* init the listview and schedule a call to listview_fill */ diff --git a/proxy.c b/proxy.c index a022a24..9c0930f 100644 --- a/proxy.c +++ b/proxy.c @@ -341,7 +341,7 @@ ProxyAuthDialogFunc(HWND hwndDlg, UINT msg, WPARAM wParam, LPARAM lParam) case WM_INITDIALOG: /* Set connection for this dialog and show it */ c = (connection_t *) lParam; - SetProp(hwndDlg, cfgProp, (HANDLE) c); + TRY_SETPROP(hwndDlg, cfgProp, (HANDLE) c); if (c->state == resuming) ForceForegroundWindow(hwndDlg); else