Hello Okan,
Here is a better explanation of where the issue lies, hopefully clarifies it
When moving any window, a small window that is child of the client's window
is created in the top-left corner and the WM works with it assuming it is kept
alive for as long as the moving/resizing lasts.
This means that if the client destroys its window, (thus destroying its childs),
cwm will continue to do operations on the destroyed window. This leads to a
crash (I assume because of X11 killing it).
The idea of the sample code was to detect the creation of the child window and
destroy the window in that moment, leading to a certain crash.
It is also reproducible by
$ sleep 5 && exit
inside a terminal emulator by starting to move the window before the exit.
Probably this would have illustrated better the issue.
Cheers
> On Sat 2024.04.06 at 15:40 +0000, ZenitDS wrote:
> > Hello,
> >
> > I sent this patch two months ago but got no response. I send it
> > again just in case.
> > Some kind of response like, "ok, we have seen it but dont want to
> > merge or mess with this", would be nice.
>
> Hi - I saw this and was at first confused.
>
> > When running cwm you can destroy the client window while
> > you are moving or resizing it, leading to crash of the WM
> > when the prop window is accessed (the one at the top-left that
> > indicates the position/scale).
> >
> > I include a demonstration for the crash and also a patch that
> > fixes it. Hopefully someone with more experience here can implement
> > a better solution.
>
> Looking again, I'm still confused as to why this sample is essentialy
> pulling events from the x event queue, rather than using XWindowEvent();
> it seems a contrived example. What is the use-case outside of this example?
>
> Best regards,
> Okan
>
> > Demonstration code for the crash:
> >
> > #include <X11/Xlib.h>
> >
> > #include <err.h>
> > #include <unistd.h>
> >
> > int
> > main(void)
> > {
> > Display *dpy;
> > Window win;
> > int scr;
> > XEvent ev;
> >
> > dpy = XOpenDisplay(NULL);
> > if (!dpy)
> > err(1, "XOpenDisplay");
> >
> > scr = DefaultScreen(dpy);
> >
> > win = XCreateSimpleWindow(dpy, RootWindow(dpy, scr), 0, 0, 500, 500, 0,
> > 0, WhitePixel(dpy, scr));
> > if (!win)
> > err(1, "XCreateSimpleWindow");
> >
> > XSelectInput(dpy, win, StructureNotifyMask | SubstructureNotifyMask |
> > ExposureMask);
> > XMapRaised(dpy, win);
> >
> > while (1) {
> > XNextEvent(dpy, &ev);
> > switch (ev.type) {
> > case CreateNotify:
> > goto end;
> > case Expose:
> > XClearWindow(dpy, win);
> > break;
> > default:
> > break;
> > }
> > }
> > end:
> > XDestroyWindow(dpy, win);
> >
> > XCloseDisplay(dpy);
> >
> > return 0;
> > }
> >
> > Here is the patch:
> >
> > Index: calmwm.h
> > ===================================================================
> > RCS file: /cvs/xenocara/app/cwm/calmwm.h,v
> > retrieving revision 1.379
> > diff -u -p -r1.379 calmwm.h
> > --- calmwm.h 20 Jul 2023 14:39:34 -0000 1.379
> > +++ calmwm.h 6 Apr 2024 15:26:37 -0000
> > @@ -481,7 +481,7 @@ struct geom screen_area(struct screen_
> > struct screen_ctx *screen_find(Window);
> > void screen_init(int);
> > void screen_prop_win_create(struct screen_ctx *,
> > Window);
> > -void screen_prop_win_destroy(struct screen_ctx *);
> > +void screen_prop_win_destroy(struct screen_ctx *,
> > int);
> > void screen_prop_win_draw(struct screen_ctx *,
> > const char *, ...)
> > __attribute__((__format__ (printf, 2, 3)))
> > @@ -558,6 +558,7 @@ void conf_screen(struct screen_ctx
> > *)
> > void conf_group(struct screen_ctx *);
> >
> > void xev_process(void);
> > +void xev_process_ev(XEvent *);
> >
> > int xu_get_prop(Window, Atom, Atom, long, unsigned
> > char **);
> > int xu_get_strprop(Window, Atom, char **);
> > Index: kbfunc.c
> > ===================================================================
> > RCS file: /cvs/xenocara/app/cwm/kbfunc.c,v
> > retrieving revision 1.174
> > diff -u -p -r1.174 kbfunc.c
> > --- kbfunc.c 20 Jul 2023 14:39:34 -0000 1.174
> > +++ kbfunc.c 6 Apr 2024 15:26:37 -0000
> > @@ -169,8 +169,8 @@ kbfunc_client_move_mb(void *ctx, struct
> >
> > screen_prop_win_create(sc, cc->win);
> > screen_prop_win_draw(sc, "%+5d%+5d", cc->geom.x, cc->geom.y);
> > - while (move) {
> > - XMaskEvent(X_Dpy, MOUSEMASK, &ev);
> > + while (move > 0) {
> > + XMaskEvent(X_Dpy, MOUSEMASK | SubstructureNotifyMask, &ev);
> > switch (ev.type) {
> > case MotionNotify:
> > /* not more than 60 times / second */
> > @@ -197,11 +197,27 @@ kbfunc_client_move_mb(void *ctx, struct
> > case ButtonRelease:
> > move = 0;
> > break;
> > + /* check for destroy events, in case the client window
> > + * gets destroyed, which forcefully closes the prop window.
> > + */
> > + case DestroyNotify:
> > + /* set move to -1 to specify abrupt exit */
> > + if (ev.xdestroywindow.window == cc->win) {
> > + screen_prop_win_destroy(sc, 1);
> > + move = -1;
> > + } else if (ev.xdestroywindow.window == sc->prop.win) {
> > + screen_prop_win_destroy(sc, 0);
> > + move = -1;
> > + }
> > + default: /* process event anyway */
> > + xev_process_ev(&ev);
> > }
> > }
> > - if (ltime)
> > - client_move(cc);
> > - screen_prop_win_destroy(sc);
> > + if (move != -1) {
> > + if (ltime)
> > + client_move(cc);
> > + screen_prop_win_destroy(sc, 1);
> > + }
> > XUngrabPointer(X_Dpy, CurrentTime);
> > }
> >
> > @@ -258,7 +274,7 @@ kbfunc_client_resize_mb(void *ctx, struc
> >
> > screen_prop_win_create(sc, cc->win);
> > screen_prop_win_draw(sc, "%4d x %-4d", cc->dim.w, cc->dim.h);
> > - while (resize) {
> > + while (resize > 0) {
> > XMaskEvent(X_Dpy, MOUSEMASK, &ev);
> > switch (ev.type) {
> > case MotionNotify:
> > @@ -277,11 +293,26 @@ kbfunc_client_resize_mb(void *ctx, struc
> > case ButtonRelease:
> > resize = 0;
> > break;
> > + /* check for destroy events, in case the client window
> > + * gets destroyed, which forcefully closes the prop window.
> > + */
> > + case DestroyNotify:
> > + if (ev.xdestroywindow.window == cc->win) {
> > + screen_prop_win_destroy(sc, 1);
> > + resize = -1;
> > + } else if (ev.xdestroywindow.window == sc->prop.win) {
> > + screen_prop_win_destroy(sc, 0);
> > + resize = -1;
> > + }
> > + default: /* process event anyway */
> > + xev_process_ev(&ev);
> > }
> > }
> > - if (ltime)
> > - client_resize(cc, 1);
> > - screen_prop_win_destroy(sc);
> > + if (resize != -1) {
> > + if (ltime)
> > + client_resize(cc, 1);
> > + screen_prop_win_destroy(sc, 1);
> > + }
> > XUngrabPointer(X_Dpy, CurrentTime);
> >
> > /* Make sure the pointer stays within the window. */
> > Index: screen.c
> > ===================================================================
> > RCS file: /cvs/xenocara/app/cwm/screen.c,v
> > retrieving revision 1.98
> > diff -u -p -r1.98 screen.c
> > --- screen.c 27 Jan 2022 18:45:10 -0000 1.98
> > +++ screen.c 6 Apr 2024 15:26:37 -0000
> > @@ -278,10 +278,15 @@ screen_prop_win_create(struct screen_ctx
> > }
> >
> > void
> > -screen_prop_win_destroy(struct screen_ctx *sc)
> > +screen_prop_win_destroy(struct screen_ctx *sc, int destroy_window)
> > {
> > XftDrawDestroy(sc->prop.xftdraw);
> > - XDestroyWindow(X_Dpy, sc->prop.win);
> > + /*
> > + * In case the window was already destroyed
> > + * (i.e. DestroyNotify event)
> > + */
> > + if (destroy_window)
> > + XDestroyWindow(X_Dpy, sc->prop.win);
> > }
> >
> > void
> > Index: xevents.c
> > ===================================================================
> > RCS file: /cvs/xenocara/app/cwm/xevents.c,v
> > retrieving revision 1.150
> > diff -u -p -r1.150 xevents.c
> > --- xevents.c 24 Mar 2020 14:47:29 -0000 1.150
> > +++ xevents.c 6 Apr 2024 15:26:37 -0000
> > @@ -485,9 +485,15 @@ xev_process(void)
> >
> > while (XPending(X_Dpy)) {
> > XNextEvent(X_Dpy, &e);
> > - if ((e.type - Conf.xrandr_event_base) == RRScreenChangeNotify)
> > - xev_handle_randr(&e);
> > - else if ((e.type < LASTEvent) && (xev_handlers[e.type] != NULL))
> > - (*xev_handlers[e.type])(&e);
> > + xev_process_ev(&e);
> > }
> > +}
> > +
> > +void
> > +xev_process_ev(XEvent *ev)
> > +{
> > + if ((ev->type - Conf.xrandr_event_base) == RRScreenChangeNotify)
> > + xev_handle_randr(ev);
> > + else if ((ev->type < LASTEvent) && (xev_handlers[ev->type] != NULL))
> > + (*xev_handlers[ev->type])(ev);
> > }
> >
