On Sat, Nov 27, 2021 at 08:30:52PM +0100, Dominik Vogt wrote: > On Sat, Nov 27, 2021 at 07:02:29PM +0100, Dominik Vogt wrote: > > There are some NULL pointer crashes and bugs telated to > > moitor_resove_ame(): > > Attempt to fix these, please proof read the patch. > > * Parsing fixes. > * monitor_resolve_name() does not crash if scr == NULL but returns > NULL. > * Callers deal with NULL beng returned. > * FScreenGetScrRect() uses the global screen f the screen is not > found. > * Export monitor_global. (A functions seems to be overkill.) > * Some other minor related cleanup. > > Doesn't crash, but I can't test it with a single monitor. One > thing to double check if whether callers should use the global, > primary or current screen if monitor_resolve_name() returns NULL. > My guesses may not be all correct.
Updated patch attached. During testing I got a crash in ewmh.c:1049 because m->Desktops of the primary screen is NULL: if ( m->Desktops->ewmh_working_area.x != x || m->Desktops->ewmh_working_area.y != y || m->Desktops->ewmh_working_area.width != width || m->Desktops->ewmh_working_area.height != height) { I think that's unrelated to the patch. It was triggered by destroying an FvwmConsole window. Program terminated with signal SIGSEGV, Segmentation fault. #0 0x000055ef2959458c in ewmh_ComputeAndSetWorkArea (m=m@entry=0x55ef2b101a20) at ewmh.c:1048 1048 if ( (gdb) bt #0 0x000055ef2959458c in ewmh_ComputeAndSetWorkArea (m=m@entry=0x55ef2b101a20) at ewmh.c:1048 #1 0x000055ef29594ebe in EWMH_WindowDestroyed () at ewmh.c:1863 #2 0x000055ef29565137 in HandleUnmapNotify (ea=<optimized out>) at events.c:3903 #3 0x000055ef29563d3e in dispatch_event (e=e@entry=0x7ffef344f560) at events.c:4185 #4 0x000055ef29564b0a in HandleEvents () at events.c:4231 #5 0x000055ef2958304f in main (argc=<optimized out>, argv=<optimized out>) at fvwm3.c:2547 Don't know how I triggered that. I was testing GotoDesk/Page/DeskAndPage with that console. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
From 7e8d87d872b571c0b7d5c1fbc024c4a8339cf1e7 Mon Sep 17 00:00:00 2001 From: Dominik Vogt <dominik.v...@gmx.de> Date: Sat, 27 Nov 2021 18:01:29 +0100 Subject: [PATCH] Fix monitor parsing II. --- fvwm/ewmh_conf.c | 9 +-- fvwm/expand.c | 4 +- fvwm/move_resize.c | 14 ++--- fvwm/placement.c | 8 ++- fvwm/virtual.c | 100 ++++++++++++++----------------- libs/FScreen.c | 83 ++++++++----------------- libs/FScreen.h | 1 + modules/FvwmIconMan/readconfig.c | 4 ++ 8 files changed, 95 insertions(+), 128 deletions(-) diff --git a/fvwm/ewmh_conf.c b/fvwm/ewmh_conf.c index cb2bb126..e94604f0 100644 --- a/fvwm/ewmh_conf.c +++ b/fvwm/ewmh_conf.c @@ -110,8 +110,7 @@ void CMD_EwmhNumberOfDesktops(F_CMD_ARGS) option = PeekToken(action, &action); if ((m = monitor_resolve_name(option)) == NULL) { - fvwm_debug(__func__, - "Invalid screen: %s", option); + fvwm_debug(__func__, "Invalid screen: %s", option); } } @@ -150,10 +149,8 @@ void CMD_EwmhBaseStruts(F_CMD_ARGS) /* Actually get the screen value. */ option = PeekToken(action, &action); - m = monitor_resolve_name(option); - if (strcmp(m->si->name, option) != 0) { - fvwm_debug(__func__, - "Invalid screen: %s", option); + if ((m = monitor_resolve_name(option)) == NULL) { + fvwm_debug(__func__, "Invalid screen: %s", option); return; } } diff --git a/fvwm/expand.c b/fvwm/expand.c index 44115ade..7aee8790 100644 --- a/fvwm/expand.c +++ b/fvwm/expand.c @@ -538,9 +538,7 @@ static signed int expand_vars_extended( rest_s = fxstrdup(rest); while ((m_name = strsep(&rest_s, ".")) != NULL) { mon2 = monitor_resolve_name(m_name); - if (m_name == NULL) - return -1; - if (strcmp(mon2->si->name, m_name) == 1) + if (mon2 == NULL) return -1; /* Skip over the monitor name. */ diff --git a/fvwm/move_resize.c b/fvwm/move_resize.c index cd070fa8..47fd322e 100644 --- a/fvwm/move_resize.c +++ b/fvwm/move_resize.c @@ -2071,11 +2071,15 @@ static void __move_window(F_CMD_ARGS, Bool do_animate, int mode) rectangle r; rectangle s; rectangle t; - struct monitor *m = monitor_get_current(); + struct monitor *m; char *token; - if (action != NULL && (token = PeekToken(action, &action)) != NULL) - m = monitor_resolve_name(token); + token = PeekToken(action, &action); + m = monitor_resolve_name(token); + if (m == NULL) + { + m = monitor_get_current(); + } s.x = m->si->x; s.y = m->si->y; @@ -3301,10 +3305,6 @@ void CMD_GeometryWindow(F_CMD_ARGS) if (token != NULL) { Scr.SizeWindow.m = monitor_resolve_name(token); - if (strcasecmp(Scr.SizeWindow.m->si->name, token) != 0) { - /* Incorrect RandR screen found. */ - Scr.SizeWindow.m = NULL; - } } } } diff --git a/fvwm/placement.c b/fvwm/placement.c index 9d081d98..0169730c 100644 --- a/fvwm/placement.c +++ b/fvwm/placement.c @@ -1732,6 +1732,8 @@ static int __place_window( if (flags.do_honor_starts_on_screen) { fscreen_scr_arg arg; + struct monitor *m; + arg.mouse_ev = NULL; /* FIXME: expand the screen name here. It's possible @@ -1762,8 +1764,12 @@ static int __place_window( * "_global" screen, which is a faked monitor for the * purposes of an older API. */ + m = NULL; if (strcmp(arg.name, "g") != 0) - fw->m = monitor_resolve_name(arg.name); + m = monitor_resolve_name(arg.name); + if (m == NULL) + m = monitor_get_current(); + fw->m = m; free(e); } else diff --git a/fvwm/virtual.c b/fvwm/virtual.c index dacb39cb..8cbcc712 100644 --- a/fvwm/virtual.c +++ b/fvwm/virtual.c @@ -1783,41 +1783,38 @@ void do_move_window_to_desk(FvwmWindow *fw, int desk) return; } -Bool get_page_arguments(FvwmWindow *fw, char *action, int *page_x, int *page_y, struct monitor **mret) +Bool get_page_arguments( + FvwmWindow *fw, char *action, int *page_x, int *page_y, + struct monitor **mret) { int val[2]; int suffix[2]; int mw, mh; char *token; - char *taction, *action_cpy, *action_cpy_start; + char *taction; + char *next; int wrapx; int wrapy; int limitdeskx; int limitdesky; - struct monitor *m, *m_use; + struct monitor *m; wrapx = 0; wrapy = 0; limitdeskx = 1; limitdesky = 1; - m_use = (fw && fw->m) ? fw->m : monitor_get_current(); - action_cpy = strdup(action); - action_cpy_start = action_cpy; - token = PeekToken(action_cpy, &action_cpy); - free(action_cpy_start); - - if (token == NULL) - return (False); - + token = PeekToken(action, &next); m = monitor_resolve_name(token); - if (strcmp(m->si->name, token) != 0) - m = m_use; + if (m != NULL) + { + action = next; + } else - PeekToken(action, &action); - - if (mret != NULL) - *mret = m; + { + m = (fw && fw->m) ? fw->m : monitor_get_current(); + } + *mret = m; mw = monitor_get_all_widths(); mh = monitor_get_all_heights(); @@ -1872,7 +1869,7 @@ Bool get_page_arguments(FvwmWindow *fw, char *action, int *page_x, int *page_y, if (GetSuffixedIntegerArguments(action, NULL, val, 2, "pw", suffix) != 2) { - return 0; + return False; } if (suffix[0] == 1) @@ -2000,7 +1997,8 @@ void parse_edge_leave_command(char *action, int type) option = PeekToken(action, &action); m = monitor_resolve_name(option); - if (strcmp(m->si->name, option) != 0) { + if (m == NULL) + { fvwm_debug(__func__, "Invalid screen: %s", option); return; @@ -2460,19 +2458,20 @@ void CMD_DesktopSize(F_CMD_ARGS) */ void CMD_GotoDesk(F_CMD_ARGS) { - struct monitor *m_use = monitor_get_current(), *m, *m_loop; - char *action_cpy, *action_cpy_start, *token; + struct monitor *m, *m_loop; + char *next, *token; int new_desk; - action_cpy = strdup(action); - action_cpy_start = action_cpy; - token = PeekToken(action_cpy, &action_cpy); - + token = PeekToken(action, &next); m = monitor_resolve_name(token); - if (strcmp(m->si->name, token) != 0) - m = m_use; + if (m != NULL) + { + action = next; + } else - PeekToken(action, &action); + { + m = monitor_get_current(); + } new_desk = GetDeskNumber(m, action, m->virtual_scr.CurrentDesk); @@ -2503,14 +2502,11 @@ void CMD_GotoDesk(F_CMD_ARGS) m_loop->virtual_scr.is_swapping = false; m->virtual_scr.is_swapping = false; - goto end; + return; } } } goto_desk(new_desk, m); -end: - free(action_cpy_start); - return; } void CMD_Desk(F_CMD_ARGS) @@ -2533,22 +2529,20 @@ void CMD_GotoDeskAndPage(F_CMD_ARGS) { int val[3]; Bool is_new_desk; - char *action_cpy, *action_cpy_start; + char *next; char *token; - struct monitor *m_use = monitor_get_current(), *m; - - action_cpy = strdup(action); - action_cpy_start = action_cpy; - token = PeekToken(action_cpy, &action_cpy); + struct monitor *m; + token = PeekToken(action, &next); m = monitor_resolve_name(token); - if (strcmp(m->si->name, token) != 0) - m = m_use; + if (m != NULL) + { + action = next; + } else - PeekToken(action, &action); - - free(action_cpy_start); - + { + m = monitor_get_current(); + } /* FIXME: monitor needs broadcast when global. */ if (MatchToken(action, "prev")) @@ -2696,20 +2690,18 @@ void CMD_Scroll(F_CMD_ARGS) int x,y; int val1, val2, val1_unit, val2_unit; char *option; + char *next; struct monitor *m = monitor_get_current(); - option = PeekToken(action, NULL); + option = PeekToken(action, &next); if (StrEquals(option, "screen")) { - /* Skip literal 'screen' */ - option = PeekToken(action, &action); /* Actually get the screen value. */ - option = PeekToken(action, &action); - + option = PeekToken(next, &action); m = monitor_resolve_name(option); - if (strcmp(m->si->name, option) != 0) { - fvwm_debug(__func__, - "Invalid screen: %s", option); - return; + if (m == NULL) + { + fvwm_debug(__func__, "Invalid screen: %s", option); + return; } } diff --git a/libs/FScreen.c b/libs/FScreen.c index 8ad08e86..8adbc1e1 100644 --- a/libs/FScreen.c +++ b/libs/FScreen.c @@ -57,7 +57,7 @@ struct screen_infos screen_info_q; struct monitors monitor_q; int randr_event; const char *prev_focused_monitor; -static struct monitor *monitor_global; +struct monitor *monitor_global = NULL; static void GetMouseXY(XEvent *eventp, int *x, int *y) { @@ -186,32 +186,25 @@ monitor_resolve_name(const char *scr) { struct monitor *m = NULL; - /* Assume the monitor name is a literal RandR name (such as HDMI2) and - * look it up regardless. - */ - m = monitor_by_name(scr); - - /* If we've asked for "@g" then use the global screen. The - * x,y,w,h values are already assigned, so skip that. - */ + if (scr == NULL) + { + return NULL; + } + /* "@g" is for the global screen. */ if (strcmp(scr, "g") == 0) { monitor_refresh_global(); m = monitor_global; } - /* "@c" is for the current screen. */ - if (strcmp(scr, "c") == 0) + else if (strcmp(scr, "c") == 0) m = monitor_get_current(); - /* "@p" is for the primary screen. */ - if (strcmp(scr, "p") == 0) + else if (strcmp(scr, "p") == 0) m = monitor_by_primary(); - - if (m == NULL) { - /* Should not happen. */ - fvwm_debug(__func__, "no monitor found with name '%s'", scr); - return (TAILQ_FIRST(&monitor_q)); - } + else + /* Assume the monitor name is a literal RandR name (such as + * HDMI2). */ + m = monitor_by_name(scr); return (m); } @@ -219,7 +212,7 @@ monitor_resolve_name(const char *scr) static struct monitor * monitor_by_name(const char *name) { - struct monitor *m, *mret = NULL; + struct monitor *m, *mret; if (name == NULL) { fvwm_debug(__func__, "%s: name is NULL; shouldn't happen. " @@ -227,6 +220,7 @@ monitor_by_name(const char *name) return (monitor_get_current()); } + mret = NULL; TAILQ_FOREACH(m, &monitor_q, entry) { if (strcmp(m->si->name, name) == 0) { mret = m; @@ -234,29 +228,6 @@ monitor_by_name(const char *name) } } - if (mret == NULL && (strcmp(name, GLOBAL_SCREEN_NAME) == 0)) { - if (monitor_get_count() == 1) { - /* In this case, the global screen was requested, but - * we've only one monitor in use. Return this monitor - * instead. - */ - return (TAILQ_FIRST(&monitor_q)); - } else { - /* Return the current monitor. */ - mret = monitor_get_current(); - } - } - - /* Then we couldn't find the named monitor at all. Return the current - * monitor instead. - */ - - if (mret == NULL) { - mret = monitor_get_current(); - if (mret == NULL) - return (NULL); - } - return (mret); } @@ -762,8 +733,8 @@ Bool FScreenGetScrRect(fscreen_scr_arg *arg, fscreen_scr_t screen, { struct monitor *m = FindScreen(arg, screen); if (m == NULL) { - fvwm_debug(__func__, "%s: m is NULL\n", __func__); - return (True); + fvwm_debug(__func__, "m is NULL, using global screen\n"); + m = monitor_global; } if (x) @@ -775,8 +746,7 @@ Bool FScreenGetScrRect(fscreen_scr_arg *arg, fscreen_scr_t screen, if (h) *h = m->si->h; - return !((monitor_get_count() > 1) && - (strcmp(m->si->name, GLOBAL_SCREEN_NAME) == 0)); + return !((monitor_get_count() > 1) && m == monitor_global); } /* Translates the coodinates *x *y from the screen specified by arg_src and @@ -995,17 +965,16 @@ int FScreenParseGeometry( parsestring, x_return, y_return, width_return, height_return, &scr); - if (scr != NULL) { - m = monitor_resolve_name(scr); - fprintf( - stderr, - "Found monitor with name of: %s (%s)\n", scr, - m->si->name); - x = m->si->x; - y = m->si->y; - w = m->si->w; - h = m->si->h; + m = monitor_resolve_name(scr); + if (m == NULL) + { + /* fall back to current screen */ + m = monitor_get_current(); } + x = m->si->x; + y = m->si->y; + w = m->si->w; + h = m->si->h; } /* adapt geometry to selected screen */ diff --git a/libs/FScreen.h b/libs/FScreen.h index 2aa16163..66559a29 100644 --- a/libs/FScreen.h +++ b/libs/FScreen.h @@ -141,6 +141,7 @@ struct monitor { TAILQ_HEAD(monitors, monitor); extern struct monitors monitor_q; +extern struct monitor *monitor_global; struct monitor *monitor_resolve_name(const char *); struct monitor *monitor_by_xy(int, int); diff --git a/modules/FvwmIconMan/readconfig.c b/modules/FvwmIconMan/readconfig.c index d593f0f4..385684cd 100644 --- a/modules/FvwmIconMan/readconfig.c +++ b/modules/FvwmIconMan/readconfig.c @@ -1203,6 +1203,10 @@ static void handle_resolution_config(int man, char *line) break; } struct monitor *mon = monitor_resolve_name(token); + if (mon == NULL) + { + mon = monitor_global; + } SET_MANAGER(man, scr, (char *)mon->si->name); line = nline; break; -- 2.30.2