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
