Fandom

Sawfish

Race condition (add window)

773pages on
this wiki
Add New Page
Talk0 Share

Ad blocker interference detected!


Wikia is a free-to-use site that makes money from advertising. We have a modified experience for viewers using ad blockers

Wikia is not accessible if you’ve made further modifications. Remove the custom ad blocker rule(s) and the page will load as expected.

Browse all patches

Author Edit

Timo Korvola

Janek Kozicki

Synopsis Edit

Note: This description is out of date, see mailing list thread for details.

Sawfish will always segfault if you create a window and instantly afterward destroy it. The timing is crucial, and if you destroy it a little "later", that is after a critical section in sawfish is passed, then sawfish will not crash. In this patch I am focusing on critical section, where some X lock would solve the problem but I don't know a way to lock X. Instead in this patch I am checking very frequently if the window was destroyed. However it could have been destroyed just after the check and before executing a following instruction which assumes that the check was successful. Therefore this patch isn't perfect. However after 200 tries I didn't manage to crash sawfish. In theory, however, sawfish is still crashable in this code. Maybe someone knows a way to lock X and prevent it from doing anything (namely - deleting a window). To stall X completely. If not, then we are left with hope that more frequent checks will result in less frequent crashes.

For me an easy way to reproduce this crash is to rename a file in rox, since I am using rox. When renaming a file two popup windows appear: 1st one asking for new file name, and 2nd indicating a renaming progress. But often the rename is so fast, that 2nd window is closed right after it appears. And this catches sawfish right in the middle of mapping a new window - a window that has just disappeared.

Another way to reproduce this bug without rox, is to write s short C program that creates a window and instantly deletes it. I recall that such a program was floating somewhere on the mailing list, but I couldn't find it now. Maybe you can find it ? :)

Patch testing Edit

  1. Copy/paste the patch listed below into some file, eg. TEST.diff.
  2. If you don't have sawfish sources yet, have one, as described get it from GIT repo.
  3. Go into the directory where sawfish sources reside, eg. cd sawfish
  4. Test if the patch applies cleanly with this command:
    patch -p1 --ignore-whitespace --dry-run < TEST.diff
    in case of problems try also: -p0 or -p2
  5. If it applies cleanly, then remove the --dry-run from above command and run it again, otherwise ask on the mailing list.
  6. Compile sawfish: ./autogen.sh && make
  7. Install it for testing, but it depends on your linux distribution.
    1. It is always better to install sawfish as your distribution package, but it is different for each distribution.
    2. So you may try make install, which will install sawifish in /usr/local/share/sawfish/ (if you have write access). But then make sure that you run the correct version and delete it from that directory afterwards, to avoid any conflicts.
  8. Se also

PS: edit this template if you feel that those instructions can be improved.

Patch Edit

--- src/windows.c_ORGINAL-1.5.0 2009-07-03 15:46:05.000000000 +0200
+++ src/windows.c       2009-10-04 17:08:50.000000000 +0200
@@ -466,22 +466,85 @@
        /* ..now do the X11 stuff */
 
        XSelectInput (dpy, id, CLIENT_EVENTS);
+       /* If the newly created window has been already deleted, the
+        * XGetWindowAttributes will tell us about it
+        * - janek kozicki */
        XGetWindowAttributes (dpy, id, &w->attr);
+       /* I assume (and HOPE) that returning invalid 'w' is OK, because on
+        * the end of this function it also returns w even if it is 0
+        * - janek kozicki */
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+       /* WARNING: 
+        * below we are entering a race-condition mode. This race condition is
+        * the cause of all those crashes that I'm trying to fix here. I don't
+        * know how to prevent X from doing anything for those few lines below.
+        * If the newly created window disappears just between one line, and
+        * another line - then sawfish will crash. 
+        * To reduce the _probability_ of this happening I am adding few
+        * more checks if (WINDOW_IS_GONE_P(w)). But window could disappear
+        * just after the check is performed, and before next instrution is
+        * executed.
+        * If someone knows how to block X from doing anything. "stall" them.
+        * Then this is the place to do it:
+        *
+        *      LOCK_X_COMPLETELY(); -> XGrabServer(dpy);
+        *
+        * After locking X, we once again need to check if WINDOW_IS_GONE_P(w):
+        *
+        *      if (WINDOW_IS_GONE_P (w))
+        *      {
+        *              UNLOCK_X_COMPLETELY(); -> XUngrabServer(dpy); Xflush(dpy);
+        *              return 0;
+        *      }
+        *
+        * NOTE: there might be some _cleaning_ necessary just before return 0
+        * from this function. So far I only discovered that 
+        *
+        *      rep_POPGC;
+        *
+        * is necesasry after call to rep_PUSHGC(gc_win, win);. But perhaps
+        * there are few others, that I'm unaware of ?
+        *
+        * - janek kozicki */
        DB(("  orig: width=%d height=%d x=%d y=%d\n",
            w->attr.width, w->attr.height, w->attr.x, w->attr.y));
 
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
        get_window_name(w);
 
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
+
        w->wmhints = XGetWMHints (dpy, id);
+
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
        if (!XGetWMNormalHints (dpy, w->id, &w->hints, &supplied))
            w->hints.flags = 0;
+
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
        get_window_protocols (w);
+
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
        if (!XGetWMColormapWindows (dpy, w->id,
                                    &w->cmap_windows, &w->n_cmap_windows))
        {
            w->n_cmap_windows = 0;
        }
 
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
+
        {
            /* Is the window shaped? */
            int xws, yws, xbs, ybs;
@@ -493,10 +556,18 @@
            w->shaped = bounding ? 1 : 0;
        }
 
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
+
        DB(("  name=`%s' x=%d y=%d width=%d height=%d\n",
            rep_STR(w->name), w->attr.x, w->attr.y,
            w->attr.width, w->attr.height));
 
+
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
        xwcm = CWX | CWX | CWWidth | CWHeight | CWBorderWidth;
        xwc.x = w->attr.x;
        xwc.y = w->attr.y;
@@ -505,12 +576,24 @@
        xwc.border_width = 0;
        XConfigureWindow (dpy, id, xwcm, &xwc);
 
+
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
         w->visible = TRUE;
        w->mapped = TRUE;               /* only called from map request */
 
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
+
        if (initialising)
            Fwindow_put (rep_VAL (w), Qplaced, Qt);
 
+       if (WINDOW_IS_GONE_P (w))
+               return 0;
+
+
        /* If the window requires to start as icon, then iconify it.
         * It is better to be done before 'before_add_window_hook', where
         * matching takes place, because in future, matcher can have an
@@ -528,10 +611,32 @@
 
        /* ..then call the add-window-hook's.. */
        rep_PUSHGC(gc_win, win);
+
+       if (WINDOW_IS_GONE_P (w))
+       {
+               rep_POPGC;
+               return 0;
+       }
+
        Fcall_window_hook (Qbefore_add_window_hook, rep_VAL(w), Qnil, Qnil);
+
+       if (WINDOW_IS_GONE_P (w))
+       {
+               rep_POPGC;
+               return 0;
+       }
+
        Fcall_window_hook (Qadd_window_hook, rep_VAL(w), Qnil, Qnil);
        rep_POPGC;
 
+
+       /* This is the end of this race condition. We are no longer in danger
+        * that sawfish could crash. If such a function exists, then this is
+        * the place to use it:
+        *      UNLOCK_X_COMPLETELY();  -> XUngrabServer(dpy); Xflush(dpy);
+        * - janek kozicki
+        */
+
        /* In case the window disappeared during the hook call */
        if (!WINDOW_IS_GONE_P (w))
        {
@@ -549,6 +654,9 @@
        else
            emit_pending_destroys ();
 
+       /* Does it mean that Fwindow_put()  is called even if there is no window 'w' ?
+        * This is just below this if(){} here. Strange.
+        * - janek kozicki */
        if (!WINDOW_IS_GONE_P (w))
        {
            repv tem = Fwindow_get (rep_VAL(w), Qplaced, Qnil);

Here's a new version of this patch, after discussions on the mailing list. A merged approach from Timo and Janek.

--- windows.c-ORG	2009-07-03 15:46:05.000000000 +0200
+++ windows.c	2009-10-09 21:59:09.000000000 +0200
@@ -459,6 +459,10 @@
 	w->net_name = Qnil;
 	w->net_icon_name = Qnil;
 
+        /* Don't garbage collect the window before we are done. */
+        /* Note: must not return without rep_POPGC. */
+	rep_PUSHGC(gc_win, win);
+
 	/* have to put it somewhere until it finds the right place */
 	insert_in_stacking_list_above_all (w);
 	restack_window (w);
@@ -525,12 +529,17 @@
 			    (rep_VAL (&iconify_mod), Qiconify_window),
 			    rep_VAL(w));
 	  }
-	
+
+		
+	/* Prevent hook call on non existing window */
+	if (WINDOW_IS_GONE_P (w))
+	{
+		rep_POPGC;
+		return 0;
+	}
 	/* ..then call the add-window-hook's.. */
-	rep_PUSHGC(gc_win, win);
 	Fcall_window_hook (Qbefore_add_window_hook, rep_VAL(w), Qnil, Qnil);
 	Fcall_window_hook (Qadd_window_hook, rep_VAL(w), Qnil, Qnil);
-	rep_POPGC;
 
 	/* In case the window disappeared during the hook call */
 	if (!WINDOW_IS_GONE_P (w))
@@ -551,13 +560,11 @@
 
 	if (!WINDOW_IS_GONE_P (w))
 	{
-           repv tem = Fwindow_get (rep_VAL(w), Qplaced, Qnil);
+            repv tem = Fwindow_get (rep_VAL(w), Qplaced, Qnil);
 	    if (initialising || (tem && tem == Qnil))
 	    {
 		/* ..then the place-window-hook.. */
-		rep_PUSHGC(gc_win, win);
 		Fcall_window_hook (Qplace_window_hook, rep_VAL(w), Qnil, Qor);
-		rep_POPGC;
 	    }
 	}
 	Fwindow_put (rep_VAL(w), Qplaced, Qt);
@@ -570,6 +577,7 @@
 	    /* Tell the window where it ended up.. */
 	    send_synthetic_configure (w);
 	}
+        rep_POPGC;
     }
     return w;
 }

Community's reasons for inclusion or rejection Edit

Patch submitters, please vote also! Yes, obviously your vote will be positive, but it's the place to give your explanation why this patch is good for all Sawfish users, and why it is correct - good reasons for inclusion.

When voting anonymously please write your name, so that it can be associated with your posts on the mailing list. If you are logged in you can sign yourself by typing four tilda characters: ~~~~.

  • Please vote with: Yes vote: yes., No vote: no., Try vote: let's try in experimental., Wtf vote: pondering. or Suspend wait for next release.
  • Yes vote: yes. I was suffering from this bug, and I fixed it to the best of my knowledge. Before this patch I was able to crash sawfish in less than 10 attempts. With this patch I tried 200 times and no crash occurred. However I would be happy if someone with greater knowledge of X an window managers mechanism would review it. And maybe add an X locking call. Janek Kozicki 16:00, October 4, 2009 (UTC)
  • Wtf vote: pondering. Seriously ugly. There has to be a better way. Tkorvola 17:24, October 4, 2009 (UTC)
Yes, that's ugly. The positive side is that sawfish crashes less often :) I'll test XGrabServer() which you suggested, when I get another chance to work on this. I'm a bit afraid that it might not work, and then what? Allow sawfish to crash? Janek Kozicki 18:20, October 4, 2009 (UTC)
OK, I've just send to mailing list a new version mixing our ideas. Janek Kozicki 20:12, October 9, 2009 (UTC)

Also on Fandom

Random Wiki