Bug 7713

Summary:

valgrind warning reading uninitialized part of xEvent when calling XWithdrawWindow (XSendEvent)

Product:

xorg

Reporter:

David Baron <dbaron>

Component:

Lib/Xlib

Assignee:

Xorg Project Team <xorg-team>

Status:

RESOLVED FIXED

QA Contact:

Xorg Project Team <xorg-team>

Severity:

normal

Priority:

high

CC:

tilman

Version:

unspecified

Keywords:

patch

Hardware:

x86 (IA32)

OS:

Linux (All)

Whiteboard:

i915 platform:

i915 features:

Attachments:

Description	Flags
patch	none
Maybe nicer patch	none

Description David Baron 2006-07-31 15:36:40 UTC

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.

Comment 1 David Baron 2007-01-17 15:25:06 UTC

Created attachment 8436 [details] [review]
patch

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).

Comment 2 John Davidorff Pell 2007-02-20 00:27:52 UTC

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!

JP

Comment 3 Tilman Sauerbeck 2007-02-20 23:29:42 UTC

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.

Comment 4 Daniel Stone 2007-02-27 01:33:03 UTC

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.

Comment 5 Tilman Sauerbeck 2007-03-28 12:22:11 UTC

Fixed in ab0bcd07957cecc8e7c0e75d5160a625e91264fe.

Comment 6 Jie Luo 2007-03-29 05:30:12 UTC

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

Comment 7 Tilman Sauerbeck 2007-03-29 07:28:00 UTC

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.

Comment 8 Tilman Sauerbeck 2007-03-29 07:32:06 UTC

Fixed the fix in 0994faa0c76c45b106442db461b8a30a3e1c9395.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.