Xlib contains a stack overflow which was discovered by Arjan van de Ven when
recompiling X.Org X11 6.8.1 with gcc4 modified with new security features that
are under development which can autodetect many common buffer overflows at
compiletime and runtime.

The stack overflow was detected at compiletime and occurs in the file
xc/lib/X11/XKBBind.c in function XkbRefreshKeyboardMapping() on line #366:


    if (event->xkb_type==XkbMapNotify) {
        XkbMapChangesRec        changes;
        Status                  rtrn;

        if (xkbi->flags&XkbMapPending)
             changes= xkbi->changes;
----->  else bzero(&changes,sizeof(XkbChangesRec));
        XkbNoteMapChanges(&changes,event,XKB_XLIB_MAP_MASK);


I investigated this issue and found the following:

The "changes" structure is of type "XkbMapChangesRec", however bzero is
being called to zero it out with a size of "sizeof(XkbChangesRec)" which
is incorrect.  Both of these structures are defined in the header
xc/include/extensions/XKBstr.h as follows:

typedef struct _XkbMapChanges {
        unsigned short           changed;
        KeyCode                  min_key_code;
        KeyCode                  max_key_code;
        unsigned char            first_type;
        unsigned char            num_types;
        KeyCode                  first_key_sym;
        unsigned char            num_key_syms;
        KeyCode                  first_key_act;
        unsigned char            num_key_acts;
        KeyCode                  first_key_behavior;
        unsigned char            num_key_behaviors;
        KeyCode                  first_key_explicit;
        unsigned char            num_key_explicit;
        KeyCode                  first_modmap_key;
        unsigned char            num_modmap_keys;
        KeyCode                  first_vmodmap_key;
        unsigned char            num_vmodmap_keys;
        unsigned char            pad;
        unsigned short           vmods;
} XkbMapChangesRec,*XkbMapChangesPtr;

typedef struct _XkbChanges {
        unsigned short           device_spec;
        unsigned short           state_changes;
        XkbMapChangesRec         map;
        XkbControlsChangesRec    ctrls;
        XkbIndicatorChangesRec   indicators;
        XkbNameChangesRec        names;
        XkbCompatChangesRec      compat;
} XkbChangesRec, *XkbChangesPtr;


Observe that the XkbChangesRec struct is much larger than the XkbMapChangesRec
struct, and actually contains a member of type XkbMapChangesRec.  As such, the
call to bzero to zero out the "changes" struct overflows, and data on the
stack is corrupted.

A preliminary examination of the effects of the overflow seem to show no
security issues would be caused by this other than application failure.

I have written a patch to fix this problem which replaces the faulty
bzero call in a manner similar to other locations in the code, as
follows:

-        bzero(&changes,sizeof(XkbChangesRec));
+        bzero(&changes,sizeof(changes));


I have not yet examined the 6.7.0 release to see if the same issue is
present, but I assume it is.  The patch I've written should be applied
to CVS HEAD, as well as the 6.8 branch (and 6.7 stable branch if the
problem exists there also), as it is a clean and obviously correct fix.

Attaching patch to fix this issue.