From dd84c2ad2fd0a55915eea17bfa60a26ccc6dfac6 Mon Sep 17 00:00:00 2001 From: Selva Nair Date: Sat, 27 Feb 2021 14:40:15 -0500 Subject: [PATCH] Remove MAX_CONFIGS limit using WM_MENUCOMMAND for menu messages We currently use WM_COMMAND message which is delivered with the ID of the menu item requiring a unique ID for every command (connect, disconnect etc..) for each connection profile. Instead, use WM_MENUCOMMAND so that the message delivers a handle to the menu and the position index of the menu item. Connection menu array is now dynamically allocated. Yet, there is still a limitation on the number of configs as the config index + mgmt_port_offset must be < 65536 to be usable as a port number. The error message shown for "too many configs" is reworded. (English language file only). Note: The current way of selecting the management port based on the index of the config file increases chances of port conflicts when the number of configs is large. It could be useful to change this logic but that is beyond the cope of this PR. Signed-off-by: Selva Nair --- main.c | 84 ++++++++++++++++++++++++--------------- openvpn_config.c | 9 +++-- options.h | 2 - res/openvpn-gui-res-en.rc | 2 +- tray.c | 81 ++++++++++++++++++++++++++----------- tray.h | 14 +++---- 6 files changed, 122 insertions(+), 70 deletions(-) diff --git a/main.c b/main.c index a8a987c..5c20c55 100644 --- a/main.c +++ b/main.c @@ -488,6 +488,8 @@ HandleCopyDataMessage(const COPYDATASTRUCT *copy_data) LRESULT CALLBACK WindowProcedure (HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam) { static UINT s_uTaskbarRestart; + int conn_id = 0; + MENUINFO minfo = {.cbSize = sizeof(MENUINFO)}; switch (message) { case WM_CREATE: @@ -532,49 +534,65 @@ LRESULT CALLBACK WindowProcedure (HWND hwnd, UINT message, WPARAM wParam, LPARAM HandleCopyDataMessage((COPYDATASTRUCT*) lParam); return TRUE; /* lets the sender free copy_data */ - case WM_COMMAND: - if ( (LOWORD(wParam) >= IDM_CONNECTMENU) && (LOWORD(wParam) < IDM_CONNECTMENU + MAX_CONFIGS) ) { - StartOpenVPN(&o.conn[LOWORD(wParam) - IDM_CONNECTMENU]); - } - if ( (LOWORD(wParam) >= IDM_DISCONNECTMENU) && (LOWORD(wParam) < IDM_DISCONNECTMENU + MAX_CONFIGS) ) { - StopOpenVPN(&o.conn[LOWORD(wParam) - IDM_DISCONNECTMENU]); - } - if ( (LOWORD(wParam) >= IDM_RECONNECTMENU) && (LOWORD(wParam) < IDM_RECONNECTMENU + MAX_CONFIGS) ) { - RestartOpenVPN(&o.conn[LOWORD(wParam) - IDM_RECONNECTMENU]); - } - if ( (LOWORD(wParam) >= IDM_STATUSMENU) && (LOWORD(wParam) < IDM_STATUSMENU + MAX_CONFIGS) ) { - ShowWindow(o.conn[LOWORD(wParam) - IDM_STATUSMENU].hwndStatus, SW_SHOW); - } - if ( (LOWORD(wParam) >= IDM_VIEWLOGMENU) && (LOWORD(wParam) < IDM_VIEWLOGMENU + MAX_CONFIGS) ) { - ViewLog(LOWORD(wParam) - IDM_VIEWLOGMENU); - } - if ( (LOWORD(wParam) >= IDM_EDITMENU) && (LOWORD(wParam) < IDM_EDITMENU + MAX_CONFIGS) ) { - EditConfig(LOWORD(wParam) - IDM_EDITMENU); - } - if ( (LOWORD(wParam) >= IDM_CLEARPASSMENU) && (LOWORD(wParam) < IDM_CLEARPASSMENU + MAX_CONFIGS) ) { - ResetSavePasswords(&o.conn[LOWORD(wParam) - IDM_CLEARPASSMENU]); - } -#ifndef DISABLE_CHANGE_PASSWORD - if ( (LOWORD(wParam) >= IDM_PASSPHRASEMENU) && (LOWORD(wParam) < IDM_PASSPHRASEMENU + MAX_CONFIGS) ) { - ShowChangePassphraseDialog(&o.conn[LOWORD(wParam) - IDM_PASSPHRASEMENU]); - } -#endif + case WM_MENUCOMMAND: + /* Get the menu item id and save it in wParam for use below */ + wParam = GetMenuItemID((HMENU) lParam, wParam); + + /* we first check global menu items which do not require a connnection index */ if (LOWORD(wParam) == IDM_IMPORT) { ImportConfigFile(); } - if (LOWORD(wParam) == IDM_SETTINGS) { + else if (LOWORD(wParam) == IDM_SETTINGS) { ShowSettingsDialog(); } - if (LOWORD(wParam) == IDM_CLOSE) { + else if (LOWORD(wParam) == IDM_CLOSE) { CloseApplication(hwnd); } - if (LOWORD(wParam) == IDM_SERVICE_START) { + else if (LOWORD(wParam) == IDM_SERVICE_START) { MyStartService(); } - if (LOWORD(wParam) == IDM_SERVICE_STOP) { + else if (LOWORD(wParam) == IDM_SERVICE_STOP) { MyStopService(); - } - if (LOWORD(wParam) == IDM_SERVICE_RESTART) MyReStartService(); + } + else if (LOWORD(wParam) == IDM_SERVICE_RESTART) { + MyReStartService(); + } + /* rest of the handlers require a connection id */ + else { + minfo.fMask = MIM_MENUDATA; + GetMenuInfo((HMENU) lParam, &minfo); + conn_id = (INT) minfo.dwMenuData; + if (conn_id < 0 || conn_id >= o.num_configs) break; /* ignore invalid connection id */ + } + + /* reach here only if the command did not match any global items and a valid connection id is available */ + + if (LOWORD(wParam) == IDM_CONNECTMENU) { + StartOpenVPN(&o.conn[conn_id]); + } + else if (LOWORD(wParam) == IDM_DISCONNECTMENU) { + StopOpenVPN(&o.conn[conn_id]); + } + else if (LOWORD(wParam) == IDM_RECONNECTMENU) { + RestartOpenVPN(&o.conn[conn_id]); + } + else if (LOWORD(wParam) == IDM_STATUSMENU) { + ShowWindow(o.conn[conn_id].hwndStatus, SW_SHOW); + } + else if (LOWORD(wParam) == IDM_VIEWLOGMENU) { + ViewLog(conn_id); + } + else if (LOWORD(wParam) == IDM_EDITMENU) { + EditConfig(conn_id); + } + else if (LOWORD(wParam) == IDM_CLEARPASSMENU) { + ResetSavePasswords(&o.conn[conn_id]); + } +#ifndef DISABLE_CHANGE_PASSWORD + else if (LOWORD(wParam) == IDM_PASSPHRASEMENU) { + ShowChangePassphraseDialog(&o.conn[conn_id]); + } +#endif break; case WM_CLOSE: diff --git a/openvpn_config.c b/openvpn_config.c index 347377a..a169115 100644 --- a/openvpn_config.c +++ b/openvpn_config.c @@ -403,6 +403,7 @@ BuildFileList() int recurse_depth = 20; /* maximum number of levels below config_dir to recurse into */ int flags = 0; int root = 0; + int max_configs = (1<<16) - o.mgmt_port_offset; if (o.silent_connection) issue_warnings = false; @@ -437,12 +438,12 @@ BuildFileList() if (o.num_configs == 0 && issue_warnings) ShowLocalizedMsg(IDS_NFO_NO_CONFIGS, o.config_dir, o.global_config_dir); - /* More than MAX_CONFIGS are ignored in the menu listing */ - if (o.num_configs > MAX_CONFIGS) + /* More than max_configs are ignored in the menu listing */ + if (o.num_configs > max_configs) { if (issue_warnings) - ShowLocalizedMsg(IDS_ERR_MANY_CONFIGS, o.num_configs); - o.num_configs = MAX_CONFIGS; /* menus don't work with more -- ignore the rest */ + ShowLocalizedMsg(IDS_ERR_MANY_CONFIGS, max_configs); + o.num_configs = max_configs; /* management-port cant handle more -- ignore the rest */ } /* if adding groups, activate non-empty ones */ diff --git a/options.h b/options.h index 072ecb3..fb56569 100644 --- a/options.h +++ b/options.h @@ -42,8 +42,6 @@ typedef struct connection connection_t; * including the option name itself. */ #define MAX_PARMS 5 /* Max number of parameters per option */ -/* Menu ids are constructed as 12 bits for config number, 4 bits for action */ -#define MAX_CONFIGS (1<<12) typedef enum { service_noaccess = -1, diff --git a/res/openvpn-gui-res-en.rc b/res/openvpn-gui-res-en.rc index 38c411d..80e9fc9 100644 --- a/res/openvpn-gui-res-en.rc +++ b/res/openvpn-gui-res-en.rc @@ -293,7 +293,7 @@ BEGIN IDS_ERR_START_CONF_EDITOR "Error starting config-editor: %s" /* OpenVPN */ - IDS_ERR_MANY_CONFIGS "OpenVPN GUI does not support more than %d configs. Please contact the author if you have the need for more." + IDS_ERR_MANY_CONFIGS "For the given parameter set, OpenVPN GUI can not support more than %d configs. Configs beyond this number are ignored." IDS_NFO_NO_CONFIGS "No readable connection profiles (config files) found.\n"\ "Use the ""Import File.."" menu or copy your config files to ""%s"" or ""%s""." IDS_ERR_CONFIG_NOT_AUTHORIZED "Starting this connection (%s) requires membership in "\ diff --git a/tray.c b/tray.c index 2c32438..59e51a6 100644 --- a/tray.c +++ b/tray.c @@ -42,7 +42,8 @@ /* Popup Menus */ HMENU hMenu; -HMENU hMenuConn[MAX_CONFIGS]; +HMENU *hMenuConn; +int hmenu_size = 0; /* allocated size of hMenuConn array */ HMENU hMenuService; HBITMAP hbmpConnecting; @@ -71,6 +72,27 @@ CreateBitmaps() } } +/* + * Reallocate hMenuConn as required. In case of not enough memory, + * o.num_config is reset to max space available value so that the + * program can continue. + */ +void +AllocateConnectionMenu() +{ + if (hmenu_size >= o.num_configs) return; + HMENU *tmp = (HMENU *) realloc(hMenuConn, sizeof(HMENU)*(o.num_configs + 50)); + if (tmp) { + hmenu_size = o.num_configs + 50; + hMenuConn = tmp; + } + else { + o.num_configs = hmenu_size; + MsgToEventLog(EVENTLOG_ERROR_TYPE, L"Allocation of hMenuConn failed. Ignoring configs beyond index = %d", o.num_configs); + } + return; +} + /* Create popup menus */ void CreatePopupMenus() @@ -81,11 +103,18 @@ CreatePopupMenus() */ assert(o.num_groups > 0); + AllocateConnectionMenu(); + CreateBitmaps(); + MENUINFO minfo = {.cbSize = sizeof(MENUINFO)}; for (int i = 0; i < o.num_configs; i++) { hMenuConn[i] = CreatePopupMenu(); + /* Save the connection index in the menu.*/ + minfo.fMask = MIM_MENUDATA; + minfo.dwMenuData = i; + SetMenuInfo(hMenuConn[i], &minfo); } for (int i = 0; i < o.num_groups; i++) { @@ -98,6 +127,12 @@ CreatePopupMenus() hMenuService = CreatePopupMenu(); hMenu = o.groups[0].menu; /* the first group menu is also the root menu */ + /* Set notify by position style for the top menu - gets automatically applied to sub-menus */ + minfo.fMask = MIM_STYLE; + GetMenuInfo(hMenu, &minfo); + minfo.dwStyle |= MNS_NOTIFYBYPOS; + SetMenuInfo(hMenu, &minfo); + if (o.num_configs == 1) { /* Create Main menu with actions */ if (o.service_only == 0) { @@ -190,21 +225,21 @@ CreatePopupMenus() /* Create popup menus for every connection */ for (int i = 0; i < o.num_configs; i++) { if (o.service_only == 0) { - AppendMenu(hMenuConn[i], MF_STRING, IDM_CONNECTMENU + i, LoadLocalizedString(IDS_MENU_CONNECT)); - AppendMenu(hMenuConn[i], MF_STRING, IDM_DISCONNECTMENU + i, LoadLocalizedString(IDS_MENU_DISCONNECT)); - AppendMenu(hMenuConn[i], MF_STRING, IDM_RECONNECTMENU + i, LoadLocalizedString(IDS_MENU_RECONNECT)); - AppendMenu(hMenuConn[i], MF_STRING, IDM_STATUSMENU + i, LoadLocalizedString(IDS_MENU_STATUS)); + AppendMenu(hMenuConn[i], MF_STRING, IDM_CONNECTMENU, LoadLocalizedString(IDS_MENU_CONNECT)); + AppendMenu(hMenuConn[i], MF_STRING, IDM_DISCONNECTMENU, LoadLocalizedString(IDS_MENU_DISCONNECT)); + AppendMenu(hMenuConn[i], MF_STRING, IDM_RECONNECTMENU, LoadLocalizedString(IDS_MENU_RECONNECT)); + AppendMenu(hMenuConn[i], MF_STRING, IDM_STATUSMENU, LoadLocalizedString(IDS_MENU_STATUS)); AppendMenu(hMenuConn[i], MF_SEPARATOR, 0, 0); } - AppendMenu(hMenuConn[i], MF_STRING, IDM_VIEWLOGMENU + i, LoadLocalizedString(IDS_MENU_VIEWLOG)); + AppendMenu(hMenuConn[i], MF_STRING, IDM_VIEWLOGMENU, LoadLocalizedString(IDS_MENU_VIEWLOG)); - AppendMenu(hMenuConn[i], MF_STRING, IDM_EDITMENU + i, LoadLocalizedString(IDS_MENU_EDITCONFIG)); - AppendMenu(hMenuConn[i], MF_STRING, IDM_CLEARPASSMENU + i, LoadLocalizedString(IDS_MENU_CLEARPASS)); + AppendMenu(hMenuConn[i], MF_STRING, IDM_EDITMENU, LoadLocalizedString(IDS_MENU_EDITCONFIG)); + AppendMenu(hMenuConn[i], MF_STRING, IDM_CLEARPASSMENU, LoadLocalizedString(IDS_MENU_CLEARPASS)); #ifndef DISABLE_CHANGE_PASSWORD if (o.conn[i].flags & FLAG_ALLOW_CHANGE_PASSPHRASE) - AppendMenu(hMenuConn[i], MF_STRING, IDM_PASSPHRASEMENU + i, LoadLocalizedString(IDS_MENU_PASSPHRASE)); + AppendMenu(hMenuConn[i], MF_STRING, IDM_PASSPHRASEMENU, LoadLocalizedString(IDS_MENU_PASSPHRASE)); #endif SetMenuStatusById(i, o.conn[i].state); @@ -515,29 +550,29 @@ SetMenuStatusById(int i, conn_state_t state) if (state == disconnected) { - EnableMenuItem(hMenuConn[i], IDM_CONNECTMENU + i, MF_ENABLED); - EnableMenuItem(hMenuConn[i], IDM_DISCONNECTMENU + i, MF_GRAYED); - EnableMenuItem(hMenuConn[i], IDM_RECONNECTMENU + i, MF_GRAYED); - EnableMenuItem(hMenuConn[i], IDM_STATUSMENU + i, MF_GRAYED); + EnableMenuItem(hMenuConn[i], IDM_CONNECTMENU, MF_ENABLED); + EnableMenuItem(hMenuConn[i], IDM_DISCONNECTMENU, MF_GRAYED); + EnableMenuItem(hMenuConn[i], IDM_RECONNECTMENU, MF_GRAYED); + EnableMenuItem(hMenuConn[i], IDM_STATUSMENU, MF_GRAYED); } else if (state == connecting || state == resuming || state == connected) { - EnableMenuItem(hMenuConn[i], IDM_CONNECTMENU + i, MF_GRAYED); - EnableMenuItem(hMenuConn[i], IDM_DISCONNECTMENU + i, MF_ENABLED); - EnableMenuItem(hMenuConn[i], IDM_RECONNECTMENU + i, MF_ENABLED); - EnableMenuItem(hMenuConn[i], IDM_STATUSMENU + i, MF_ENABLED); + EnableMenuItem(hMenuConn[i], IDM_CONNECTMENU, MF_GRAYED); + EnableMenuItem(hMenuConn[i], IDM_DISCONNECTMENU, MF_ENABLED); + EnableMenuItem(hMenuConn[i], IDM_RECONNECTMENU, MF_ENABLED); + EnableMenuItem(hMenuConn[i], IDM_STATUSMENU, MF_ENABLED); } else if (state == disconnecting) { - EnableMenuItem(hMenuConn[i], IDM_CONNECTMENU + i, MF_GRAYED); - EnableMenuItem(hMenuConn[i], IDM_DISCONNECTMENU + i, MF_GRAYED); - EnableMenuItem(hMenuConn[i], IDM_RECONNECTMENU + i, MF_GRAYED); - EnableMenuItem(hMenuConn[i], IDM_STATUSMENU + i, MF_ENABLED); + EnableMenuItem(hMenuConn[i], IDM_CONNECTMENU, MF_GRAYED); + EnableMenuItem(hMenuConn[i], IDM_DISCONNECTMENU, MF_GRAYED); + EnableMenuItem(hMenuConn[i], IDM_RECONNECTMENU, MF_GRAYED); + EnableMenuItem(hMenuConn[i], IDM_STATUSMENU, MF_ENABLED); } if (c->flags & (FLAG_SAVE_AUTH_PASS | FLAG_SAVE_KEY_PASS)) - EnableMenuItem(hMenuConn[i], IDM_CLEARPASSMENU + i, MF_ENABLED); + EnableMenuItem(hMenuConn[i], IDM_CLEARPASSMENU, MF_ENABLED); else - EnableMenuItem(hMenuConn[i], IDM_CLEARPASSMENU + i, MF_GRAYED); + EnableMenuItem(hMenuConn[i], IDM_CLEARPASSMENU, MF_GRAYED); } } diff --git a/tray.h b/tray.h index 7aa6924..5250d5f 100644 --- a/tray.h +++ b/tray.h @@ -34,13 +34,13 @@ #define IDM_IMPORT 224 #define IDM_CONNECTMENU 300 -#define IDM_DISCONNECTMENU (MAX_CONFIGS + IDM_CONNECTMENU) -#define IDM_STATUSMENU (MAX_CONFIGS + IDM_DISCONNECTMENU) -#define IDM_VIEWLOGMENU (MAX_CONFIGS + IDM_STATUSMENU) -#define IDM_EDITMENU (MAX_CONFIGS + IDM_VIEWLOGMENU) -#define IDM_PASSPHRASEMENU (MAX_CONFIGS + IDM_EDITMENU) -#define IDM_CLEARPASSMENU (MAX_CONFIGS + IDM_PASSPHRASEMENU) -#define IDM_RECONNECTMENU (MAX_CONFIGS + IDM_CLEARPASSMENU) +#define IDM_DISCONNECTMENU (1 + IDM_CONNECTMENU) +#define IDM_STATUSMENU (1 + IDM_DISCONNECTMENU) +#define IDM_VIEWLOGMENU (1 + IDM_STATUSMENU) +#define IDM_EDITMENU (1 + IDM_VIEWLOGMENU) +#define IDM_PASSPHRASEMENU (1 + IDM_EDITMENU) +#define IDM_CLEARPASSMENU (1 + IDM_PASSPHRASEMENU) +#define IDM_RECONNECTMENU (1 + IDM_CLEARPASSMENU) void CreatePopupMenus(); void OnNotifyTray(LPARAM);