When I close a modal dialog in Firefox (running with --sync), I get the
following valgrind warning (trimmed addresses to fit neatly):
Syscall param write(buf) points to uninitialised byte(s)
at (within /lib/libpthread-2.4.so)
by _X11TransWrite (/usr/include/X11/Xtrans/Xtrans.c:897)
by _XFlushInt (/usr/src/debug/libX11-1.0.0/src/XlibInt.c:653)
by _XReply (/usr/src/debug/libX11-1.0.0/src/XlibInt.c:1692)
by XSync (/usr/src/debug/libX11-1.0.0/src/Sync.c:48)
by _XSyncFunction (/usr/src/debug/libX11-1.0.0/src/Synchro.c:37)
by XSendEvent (/usr/src/debug/libX11-1.0.0/src/SendEvent.c:77)
by XWithdrawWindow (/usr/src/debug/libX11-1.0.0/src/Withdraw.c:81)
by gdk_window_withdraw (/builds/gtk2/cvs-gnome/gtk+/gdk/x11/gdkwindow-x11.c:1636)
This appears to be because XSendEvent sends an entire xEvent struct as part of
the event, but _XEventToWire (I presume) has only filled in the non-padding
parts of the xEvent.u.unmapNotify.
It would be good to zero-fill the padding in the event so that uninitialized
data is not written to a socket. This avoids spurious valgrind warnings when
running programs that use X -- warnings that are rather hard to filter out when
not running with --sync.
Created attachment 8436 [details] [review]
I tested that this patch fixes the valgrind warnings from Mozilla's session
restore dialog in libX11 pulled from git this morning, built with --without-xcb
(since xsltproc wouldn't behave during xcb compilation).
This also has the added benefit of allowing compressed X connections (i.e. over ssh w/ compression enabled) to more succinctly compress the zero-filled bytes. :-) Yay!
Created attachment 8796 [details] [review]
Maybe nicer patch
To make this patch look less like dangerous, we might want to move the memset() to the caller of _XEventToWire(), which is XSendEvent(). Like this. Untested.
Sorry about the phenomenal bug spam, guys. Adding xorg-team@ to the QA contact so bugs don't get lost in future.
Fixed in ab0bcd07957cecc8e7c0e75d5160a625e91264fe.
I think the prototype of memset is
void *memset(void *s, int c, size_t n)
So maybe this patch should change to
memset (&ev, 0, sizeof(ev));
Man, I suck.
I knew that error was in the patch that I attached, but I didn't bother to mention it here since it's so obvious. I also tested the modified version, but when I committed the patch, I grabbed the original from bugzilla.
Thanks for spotting that, Jie Luo.
Fixed the fix in 0994faa0c76c45b106442db461b8a30a3e1c9395.