Re: [PATCH] Re: Crashes with monitor_resolve_name
Updated patch in dv/monitor-parsing-fix. Doesn't take care of the NULL m->Desktops crash. Don't know how to reproduce. Ciao Dominik ^_^ ^_^ -- Dominik Vogt
Re: [PATCH] Re: Crashes with monitor_resolve_name
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 0x55ef2959458c in ewmh_ComputeAndSetWorkArea (m=m@entry=0x55ef2b101a20)
at ewmh.c:1048
1048if (
(gdb) bt
#0 0x55ef2959458c in ewmh_ComputeAndSetWorkArea (m=m@entry=0x55ef2b101a20)
at ewmh.c:1048
#1 0x55ef29594ebe in EWMH_WindowDestroyed () at ewmh.c:1863
#2 0x55ef29565137 in HandleUnmapNotify (ea=)
at events.c:3903
#3 0x55ef29563d3e in dispatch_event (e=e@entry=0x7ffef344f560)
at events.c:4185
#4 0x55ef29564b0a in HandleEvents () at events.c:4231
#5 0x55ef2958304f in main (argc=, argv=)
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
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, );
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, );
- 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(_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, )) != NULL)
- m = monitor_resolve_name(token);
+ token = PeekToken(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
---
[PATCH] Re: Crashes with monitor_resolve_name
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.
Ciao
Dominik ^_^ ^_^
--
Dominik Vogt
From 288bfd7f95a87bdeb5783d5d4a7afcd1926680d0 Mon Sep 17 00:00:00 2001
From: Dominik Vogt
Date: Sat, 27 Nov 2021 18:01:29 +0100
Subject: [PATCH] Fix monitor parsing.
---
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 | 37 ++--
libs/FScreen.h | 1 +
modules/FvwmIconMan/readconfig.c | 4 ++
8 files changed, 87 insertions(+), 90 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, );
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, );
- 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(_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, )) != NULL)
- m = monitor_resolve_name(token);
+ token = PeekToken(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;
Re: Crashes with monitor_resolve_name
On Sat, Nov 27, 2021 at 07:02:29PM +0100, Dominik Vogt wrote: > Are there other places with unusual monitor name parsing? Maybe in GotoDeskAndPage, but I don't think there's other places. -- Thomas
Crashes with monitor_resolve_name
There are some NULL pointer crashes and bugs telated to
moitor_resove_ame():
1. The fuction assumes that it's always called with a non-null
"scr" pointer:
monitor_resolve_name(const char *scr)
{
...
if (strcmp(scr, "g") == 0) {
This is not the case. Several places in the source call the
function without validating that the pointer in non-NULL.
This can be triggered with the command "desk" (no arguments).
2. #1 is because bugs in the parsing code that may be the result
of a misunderstanding how the parsing functions work:
-- snip --
01 void CMD_GotoDesk(F_CMD_ARGS)
02 {
03 ...
04 action_cpy = strdup(action);
05 action_cpy_start = action_cpy;
06 token = PeekToken(action_cpy, _cpy);
07
08 m = monitor_resolve_name(token);
09 if (strcmp(m->si->name, token) != 0)
10 m = m_use;
11 else
12 PeekToken(action, );
13
14 new_desk = GetDeskNumber(m, action, m->virtual_scr.CurrentDesk);
-- snip --
Problems here:
line 02 + 03: This is pointless. Neither PeekToken nor the other
parsing functions modify the "action".
line 04: fxstrdup() should be used.
line 06: This seems to assume that PeekToken always returns a
string pointer. This is *not* the case! The parsing
functions never return an empty string.
Also, if the caller is not interested in the rest of the
command line, just pass NULL as second argument:
token = PeekToken(action, NULL);
line 08: Crashes because of #1.
line 09: Crashes because token can be NULL.
line 12: The proper way to skip tokens at the beginning of a
string is
action = SkipNTokens(action, 1);
line 14: OK because GetDeskNumber uses MatchToken, which is aware
of all possible NULL pointers.
--
Also, there's a real parsing bug:
If called with "desk arg1 arg2":
Case 1: arg1 is a monitor name.
-> calls GetDeskNumberGetDeskNumber with action = "arg1 arg2"
^^^
Case 2: arg1 is no monitor name.
-> calls GetDeskNumberGetDeskNumber with action = "arg2"
^^
That can't be correct.
--
(Specifying a monitor name is not documented in the man page).
--
Are there other places with unusual monitor name parsing?
Ciao
Dominik ^_^ ^_^
--
Dominik Vogt
