Re: NEWS on 1.6.0, crash fix... semi-fix (if you create and quickly delete a window sawfish will segfault)

[Janek Kozicki <janek_listy wp pl>]

Timo Korvola said:     (by the date of Thu, 8 Oct 2009 00:52:51 +0300)

> My approach there is not mutually exclusive with Janek's, which 
> consisted of adding more error checking to add_window.  To test my patch 
> one should try to trip Sawfish on errors as hard as possible, so it is 
> best not to add any error checking before gaining reasonable confidence 
> that my patch works.  However, eventually some more intermediate error 
> checking in add_window would perhaps be good, although not quite as much 
> as in Janek's patch should be necessary.


Hi, so I did extensive testing.


With Timo's patch I did 130 tries and no crash! However Following valgrind error
occurs once, at the FIRST try:

==11117== Conditional jump or move depends on uninitialised value(s)
==11117==    at 0x4E5EFE3: vm (lispmach.h:850)
==11117==    by 0x4E5DE8C: inline_apply_bytecode (lispmach.h:505)
==11117==    by 0x4E5EC2B: vm (lispmach.h:751)
==11117==    by 0x4E5DE8C: inline_apply_bytecode (lispmach.h:505)
==11117==    by 0x4E646F0: rep_apply_bytecode (lispmach.c:81)
==11117==    by 0x4E53CE9: apply (lisp.c:1710)
==11117==    by 0x4E53F98: Ffuncall (lisp.c:1776)
==11117==    by 0x4E5CC27: Fcall_hook (lispcmds.c:1934)
==11117==    by 0x42A121: Fcall_window_hook (windows.c:1374)
==11117==    by 0x42B03F: add_window (windows.c:534)
==11117==    by 0x415B0F: map_request (events.c:751)
==11117==    by 0x414501: inner_handle_input (events.c:1406)
==11117== 

We can see that windows.c:534 calls Fcall_window_hook() with
an uninitialised argument. So I tired adding my safegueard just before
this call, and so then I tried 130 times, and valgrind didn't produce
any error at all! And no crash too. Instead sawfish was printing this
error message:

  warning: failed to allocate a window

Which is much better than valgrind reporting errors :)

So I guess that attached patch against 1.5.0 is the best option.



PS: As a side note - the XGrabServer method didn't work at all.
Instant crash on first try, even though XGrabServer() indeed grabs
server! I tested that it indeed grabs server with simple attached program :)

-- 
Janek Kozicki                                                         |

--- 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;
 }

// g++ click.cpp -o klik -lXtst -lXext -lX11
#include<X11/extensions/XTest.h>
#include<iostream>
#include<stdlib.h>
#include<boost/lexical_cast.hpp>
#include <unistd.h>

static Display *TheXDisplay = NULL;

int main(int argc, char** argv)
{
	TheXDisplay = XOpenDisplay(":2");
	if (TheXDisplay == NULL)
	{
		std::cerr <<"This program is designed to run in X Windows!\n";
		exit(1);
	}
	int x = boost::lexical_cast<int>(argv[1]);
	int y = boost::lexical_cast<int>(argv[2]);
	std::cerr << "click: " << x << "," << y << "\n";
	XSync(TheXDisplay, True);

XGrabServer(TheXDisplay);

	XTestFakeMotionEvent(TheXDisplay,0, x, y, 0);
	XTestFakeButtonEvent(TheXDisplay,1, true, 0);
	XTestFakeButtonEvent(TheXDisplay,1, false, 0);
	sleep(10);

XUngrabServer(TheXDisplay); XFlush(TheXDisplay);

//	XFlush(TheXDisplay);
	XSync(TheXDisplay, False);
	XCloseDisplay(TheXDisplay);
	TheXDisplay = NULL;
};