[PATCH] valgrind warning fixes in libX11
L. David Baron
dbaron at dbaron.org
Tue Feb 20 15:47:21 PST 2007
[ resending without GPG signature, which (combined with attachments)
confused the list archive and perhaps some email clients ]
The two attached patches to libX11 fix valgrind warnings that I see
starting Mozilla. The warnings are "syscall param write(buf) points
to uninitialised byte(s)", with a stack showing data being sent to
the X server.
They are the only patches needed to get trunk Firefox (as of a few
weeks ago) to start up without valgrind warnings at least some of
the time (other than the warnings in the suppressions file that
ships with valgrind on Fedora Core 6). (The problem that causes
warnings the remainder of the time is a Mozilla bug.)
I think it would be quite useful for these bugs to be fixed in
libX11, since it's basically impossible to suppress the warnings
they cause without suppressing all warnings about sending
uninitialized data to the X server unless you make everybody using
valgrind run apps with --sync. Fixing them would make it easier to
catch cases where client apps are sending uninitialized data to the
X server. (Actually debugging such issues requires --sync, but
noticing them does not. Or at least, noticing them would not
require --sync if these patches were incorporated into libX11, since
otherwise real warnings cannot be distinguished from the warnings
caused by the bugs patched here.)
There are two patches attached:
(1) The first patch, previously attached to
https://bugs.freedesktop.org/show_bug.cgi?id=7703 , changes the
functions XSetSizeHints, XSetWMHints, and XSetWMSizeHints so
they only send to the server the fields that the client
indicated are present (and send zero-filled data for the
remainder of the structure).
I think the pattern I'm changing the code to use is better than
the existing code. In particular, if the libX11 structs passed
to these functions were ever extended, failing to do what my
patch does, at least for the new members, could cause the
reading of unallocated memory (which could potentially crash
given an allocation near a page boundary).
However, I'm in no position to judge potential compatibility
issues with this patch. I'm also not entirely sure that my
handling of the USSize and USPosition bits is correct.
(2) The second patch, previously attached to
https://bugs.freedesktop.org/show_bug.cgi?id=7713 ,
zero-initializes the xEvent structure in _XEventToWire, which
does not fully initialize the structure for many types of events
(since it is a union).
I'm not sure what the process for getting patches considered is, but
since attaching them to the bugs didn't work, I'm writing this
email.
-David
--
L. David Baron <URL: http://dbaron.org/ >
Technical Lead, Layout & CSS, Mozilla Corporation
-------------- next part --------------
diff --git a/src/SetHints.c b/src/SetHints.c
index 5ee3443..d32ebba 100644
--- a/src/SetHints.c
+++ b/src/SetHints.c
@@ -66,22 +66,35 @@ XSetSizeHints(dpy, w, hints, property) /* old routine */
XSizeHints *hints;
Atom property;
{
- xPropSizeHints prop;
+ xPropSizeHints prop;
+ memset(&prop, 0, sizeof(prop));
prop.flags = (hints->flags & (USPosition|USSize|PAllHints));
- prop.x = hints->x;
- prop.y = hints->y;
- prop.width = hints->width;
- prop.height = hints->height;
- prop.minWidth = hints->min_width;
- prop.minHeight = hints->min_height;
- prop.maxWidth = hints->max_width;
- prop.maxHeight = hints->max_height;
- prop.widthInc = hints->width_inc;
- prop.heightInc = hints->height_inc;
- prop.minAspectX = hints->min_aspect.x;
- prop.minAspectY = hints->min_aspect.y;
- prop.maxAspectX = hints->max_aspect.x;
- prop.maxAspectY = hints->max_aspect.y;
+ if (hints->flags & (USPosition|PPosition)) { /* XXX ? */
+ prop.x = hints->x;
+ prop.y = hints->y;
+ }
+ if (hints->flags & (USSize|PSize)) { /* XXX ? */
+ prop.width = hints->width;
+ prop.height = hints->height;
+ }
+ if (hints->flags & PMinSize) {
+ prop.minWidth = hints->min_width;
+ prop.minHeight = hints->min_height;
+ }
+ if (hints->flags & PMaxSize) {
+ prop.maxWidth = hints->max_width;
+ prop.maxHeight = hints->max_height;
+ }
+ if (hints->flags & PResizeInc) {
+ prop.widthInc = hints->width_inc;
+ prop.heightInc = hints->height_inc;
+ }
+ if (hints->flags & PAspect) {
+ prop.minAspectX = hints->min_aspect.x;
+ prop.minAspectY = hints->min_aspect.y;
+ prop.maxAspectX = hints->max_aspect.x;
+ prop.maxAspectY = hints->max_aspect.y;
+ }
return XChangeProperty (dpy, w, property, XA_WM_SIZE_HINTS, 32,
PropModeReplace, (unsigned char *) &prop,
OldNumPropSizeElements);
@@ -99,15 +112,24 @@ XSetWMHints (dpy, w, wmhints)
XWMHints *wmhints;
{
xPropWMHints prop;
+ memset(&prop, 0, sizeof(prop));
prop.flags = wmhints->flags;
- prop.input = (wmhints->input == True ? 1 : 0);
- prop.initialState = wmhints->initial_state;
- prop.iconPixmap = wmhints->icon_pixmap;
- prop.iconWindow = wmhints->icon_window;
- prop.iconX = wmhints->icon_x;
- prop.iconY = wmhints->icon_y;
- prop.iconMask = wmhints->icon_mask;
- prop.windowGroup = wmhints->window_group;
+ if (wmhints->flags & InputHint)
+ prop.input = (wmhints->input == True ? 1 : 0);
+ if (wmhints->flags & StateHint)
+ prop.initialState = wmhints->initial_state;
+ if (wmhints->flags & IconPixmapHint)
+ prop.iconPixmap = wmhints->icon_pixmap;
+ if (wmhints->flags & IconWindowHint)
+ prop.iconWindow = wmhints->icon_window;
+ if (wmhints->flags & IconPositionHint) {
+ prop.iconX = wmhints->icon_x;
+ prop.iconY = wmhints->icon_y;
+ }
+ if (wmhints->flags & IconMaskHint)
+ prop.iconMask = wmhints->icon_mask;
+ if (wmhints->flags & WindowGroupHint)
+ prop.windowGroup = wmhints->window_group;
return XChangeProperty (dpy, w, XA_WM_HINTS, XA_WM_HINTS, 32,
PropModeReplace, (unsigned char *) &prop,
NumPropWMHintsElements);
diff --git a/src/SetNrmHint.c b/src/SetNrmHint.c
index 64b0ef7..92a9e8f 100644
--- a/src/SetNrmHint.c
+++ b/src/SetNrmHint.c
@@ -71,29 +71,46 @@ void XSetWMSizeHints (dpy, w, hints, prop)
data.flags = (hints->flags &
(USPosition|USSize|PPosition|PSize|PMinSize|PMaxSize|
PResizeInc|PAspect|PBaseSize|PWinGravity));
+ memset(&data, 0, sizeof(data));
/*
* The x, y, width, and height fields are obsolete; but, applications
* that want to work with old window managers might set them.
*/
- data.x = hints->x;
- data.y = hints->y;
- data.width = hints->width;
- data.height = hints->height;
-
- data.minWidth = hints->min_width;
- data.minHeight = hints->min_height;
- data.maxWidth = hints->max_width;
- data.maxHeight = hints->max_height;
- data.widthInc = hints->width_inc;
- data.heightInc = hints->height_inc;
- data.minAspectX = hints->min_aspect.x;
- data.minAspectY = hints->min_aspect.y;
- data.maxAspectX = hints->max_aspect.x;
- data.maxAspectY = hints->max_aspect.y;
- data.baseWidth = hints->base_width;
- data.baseHeight = hints->base_height;
- data.winGravity = hints->win_gravity;
+ if (hints->flags & (USPosition|PPosition)) { /* XXX ? */
+ data.x = hints->x;
+ data.y = hints->y;
+ }
+ if (hints->flags & (USSize|PSize)) { /* XXX ? */
+ data.width = hints->width;
+ data.height = hints->height;
+ }
+
+ if (hints->flags & PMinSize) {
+ data.minWidth = hints->min_width;
+ data.minHeight = hints->min_height;
+ }
+ if (hints->flags & PMaxSize) {
+ data.maxWidth = hints->max_width;
+ data.maxHeight = hints->max_height;
+ }
+ if (hints->flags & PResizeInc) {
+ data.widthInc = hints->width_inc;
+ data.heightInc = hints->height_inc;
+ }
+ if (hints->flags & PAspect) {
+ data.minAspectX = hints->min_aspect.x;
+ data.minAspectY = hints->min_aspect.y;
+ data.maxAspectX = hints->max_aspect.x;
+ data.maxAspectY = hints->max_aspect.y;
+ }
+ if (hints->flags & PBaseSize) {
+ data.baseWidth = hints->base_width;
+ data.baseHeight = hints->base_height;
+ }
+ if (hints->flags & PWinGravity) {
+ data.winGravity = hints->win_gravity;
+ }
XChangeProperty (dpy, w, prop, XA_WM_SIZE_HINTS, 32,
PropModeReplace, (unsigned char *) &data,
-------------- next part --------------
diff --git a/src/EvToWire.c b/src/EvToWire.c
index 1433527..714842b 100644
--- a/src/EvToWire.c
+++ b/src/EvToWire.c
@@ -50,6 +50,7 @@ register Display *dpy, /* pointer to display structure */
register XEvent *re, /* pointer to where event should be reformatted */
register xEvent *event) /* wire protocol event */
{
+ memset(event, 0, SIZEOF(xEvent));
switch (event->u.u.type = re->type) {
case KeyPress:
case KeyRelease:
More information about the xorg
mailing list