[PATCH] fix xfwm4 crash on OpenBSD; time_t size problem

[Stefan Sperling]

Cc'ing xfce4-dev -- I'm subscribed here, too, though usually just lurking.
I'm leaving the context intact for reference.

The patch is at the very bottom. It is against OpenBSD's ports tree
but is trivial enough for manually applying to xfwm4 sources.

On Sat, Feb 08, 2014 at 10:13:01AM +0100, Stefan Sperling wrote:
> On Fri, Feb 07, 2014 at 07:45:09AM +0100, Landry Breuil wrote:
> > On Thu, Feb 06, 2014 at 09:45:47PM +0100, Stefan Sperling wrote:
> > > Hey Landry,
> > > 
> > > since I updated to -current i386 yesterday, XFCE's window manager
> > > dumps core every time I start firefox from the panel if startup
> > > notifications are enabled in firefox' launcher. The launcher runs
> > > 'firefox %u'.
> > > 
> > > I cannot reproduce the issue with any other application (terminal,
> > > pidgin, xchat), and I cannot reproduce it when starting firefox
> > > from the command line.
> > > 
> > > Is this a known problem?
> > 
> > nope :) but you say this only happens if startup notifications are
> > enabled ?
> 
> I've had notifications enabled forever because otherwise firefox
> steals focus each time it is started.
> 
> I can trigger the xfwm crash on or off by enabling notifications
> for the firefox launcher in the panel (xfwm will crash) and disabling
> notifications for firefox (xfwm won't crash).
> 
> I also have focus stealing prevention enabled.
> 
> > What about amd64 ?
> 
> I cannot reproduce it on amd64 so far.
> 
> Only on i386.
> But perhaps the machine I'm seeing it on has a specific problem.
> 
> > Either way, could be a real bug in xfwm4 so i'm interested if you can
> > gather more details, we'll report it upstream.
> 
> I'll try to figure out more.
> 
> BTW, I tried to find you at FOSDEM, but couldn't. Maybe next time :)

I have a diagnosis and a fix.

Xfwm4 crashes as follows: 

Program received signal SIGSEGV, Segmentation fault.
0x060da5da in sn_startup_sequence_complete (sequence=0x0) at sn-monitor.c:406
406       if (sequence->id == NULL)
Current language:  auto; currently c
(gdb) bt     
#0  0x060da5da in sn_startup_sequence_complete (sequence=0x0)
    at sn-monitor.c:406
#1  0x17da7d05 in sn_startup_sequence_timeout (data=0x866af4b0)
    at startup_notification.c:168
#2  0x06517156 in g_timeout_dispatch ()
   from /usr/local/lib/libglib-2.0.so.3800.0
#3  0x065167c4 in g_main_context_dispatch ()
   from /usr/local/lib/libglib-2.0.so.3800.0
#4  0x06518a09 in g_main_context_iterate ()
   from /usr/local/lib/libglib-2.0.so.3800.0
#5  0x06519c17 in g_main_loop_run () from /usr/local/lib/libglib-2.0.so.3800.0
#6  0x0a4bf8d4 in gtk_main () from /usr/local/lib/libgtk-x11-2.0.so.2400.0
#7  0x17d8d085 in main (argc=1, argv=0xcfbea6ac) at main.c:627
(gdb) up
#1  0x17da7d05 in sn_startup_sequence_timeout (data=0x866af4b0)
    at startup_notification.c:168
168             sn_startup_sequence_complete (sequence);
(gdb) p sequence
$1 = (SnStartupSequence *) 0x0

Somewhat confusingly, the xfwm4 functions calling into libstartup-notification
are also named with an sn_ prefix. So some sn_* functions are from the startup
lib, some from xfwm4. The sn_startup_sequence_timeout() function is from xfwm4.
sn_startup_sequence_complete() is from libstartup-notification.

The crash happens because this code is walking a corrupt list.  
The list is corrupt because xfwm4 uses pointers of the wrong size
(long instead of time_t) while constructing elements of the list.
time_t is always 64bit on OpenBSD. But 'long' is 32bit on OpenBSD/i386.
This explains why it doesn't crash on OpenBSD/amd64 where long' is 64bit.

(code from libstartup-notification)
[[[
void
sn_startup_sequence_get_last_active_time (SnStartupSequence *sequence,
                                          time_t            *tv_sec,
                                          suseconds_t       *tv_usec)
{
  /* for now the same as get_initiated_time */
  if (tv_sec)
    *tv_sec = sequence->initiation_time.tv_sec;
  if (tv_usec)
    *tv_usec = sequence->initiation_time.tv_usec;
}       
]]]

(code from xfwm4)
[[[
static void
sn_collect_timed_out_foreach (void *element, void *data)
{
    CollectTimedOutData *ctod;
    SnStartupSequence *sequence;
    long tv_sec, tv_usec;
    double elapsed;

    g_return_if_fail (data != NULL);
    g_return_if_fail (element != NULL);

    sequence = element;
    ctod = (CollectTimedOutData *) data;
    sn_startup_sequence_get_last_active_time (sequence, &tv_sec, &tv_usec);

    elapsed =
        ((((double) ctod->now.tv_sec - tv_sec) * G_USEC_PER_SEC +
            (ctod->now.tv_usec - tv_usec))) / 1000.0;

    if (elapsed > STARTUP_TIMEOUT)
    {
        ctod->list = g_slist_prepend (ctod->list, sequence);
    }
}
]]]

The above call to sn_startup_sequence_get_last_active_time() probably
corrupts the value of the 'sequence' pointer on the stack, which is
then copied into the list.

This patch fixes the problem for me.

Index: Makefile
===================================================================
RCS file: /cvs/ports/x11/xfce4/xfwm4/Makefile,v
retrieving revision 1.42
diff -u -p -r1.42 Makefile
--- Makefile	31 May 2013 19:21:10 -0000	1.42
+++ Makefile	8 Feb 2014 10:25:45 -0000
@@ -3,6 +3,7 @@
 COMMENT=	Xfce4 window manager
 
 XFCE_PROJECT=	xfwm4
+REVISION=	0
 
 # GPLv2
 PERMIT_PACKAGE_CDROM=	Yes
Index: patches/patch-src_startup_notification_c
===================================================================
RCS file: patches/patch-src_startup_notification_c
diff -N patches/patch-src_startup_notification_c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ patches/patch-src_startup_notification_c	8 Feb 2014 10:09:17 -0000
@@ -0,0 +1,13 @@
+$OpenBSD$
+--- src/startup_notification.c.orig	Sat Feb  8 11:08:22 2014
++++ src/startup_notification.c	Sat Feb  8 11:08:13 2014
+@@ -126,7 +126,8 @@ sn_collect_timed_out_foreach (void *element, void *dat
+ {
+     CollectTimedOutData *ctod;
+     SnStartupSequence *sequence;
+-    long tv_sec, tv_usec;
++    time_t tv_sec;
++    suseconds_t tv_usec;
+     double elapsed;
+ 
+     g_return_if_fail (data != NULL);