[PATCH] xkb: fix invalid memory writes in _XkbCopyGeom.

[Peter Hutterer]

On Fri, Jun 04, 2010 at 08:47:05AM +0300, Oliver McFadden wrote:
> On Fri, 2010-06-04 at 07:41 +0200, ext Peter Hutterer wrote:
> > On Fri, Jun 04, 2010 at 02:15:04PM +1000, Peter Hutterer wrote:
> > > Classic strlen/strcpy mistake of
> > >    foo = malloc(strlen(bar));
> > >    strcpy(foo, bar);
> > > 
> > > Testcase: valgrind Xephyr :1
> > > 
> > > ==8591== Invalid write of size 1
> > > ==8591==    at 0x4A0638F: strcpy (mc_replace_strmem.c:311)
> > > ==8591==    by 0x605593: _XkbCopyGeom (xkbUtils.c:1994)
> > > ==8591==    by 0x605973: XkbCopyKeymap (xkbUtils.c:2118)
> > > ==8591==    by 0x6122B3: InitKeyboardDeviceStruct (xkbInit.c:560)
> > > ==8591==    by 0x4472E2: CoreKeyboardProc (devices.c:577)
> > > ==8591==    by 0x447162: ActivateDevice (devices.c:530)
> > > ==8591==    by 0x4475D6: InitCoreDevices (devices.c:672)
> > > ==8591==    by 0x4449EE: main (main.c:254)
> > > ==8591==  Address 0x6f96505 is 0 bytes after a block of size 53 alloc'd
> > > ==8591==    at 0x4A0515D: malloc (vg_replace_malloc.c:195)
> > > ==8591==    by 0x6054B7: _XkbCopyGeom (xkbUtils.c:1980)
> > > ==8591==    by 0x605973: XkbCopyKeymap (xkbUtils.c:2118)
> > > ==8591==    by 0x6122B3: InitKeyboardDeviceStruct (xkbInit.c:560)
> > > ==8591==    by 0x4472E2: CoreKeyboardProc (devices.c:577)
> > > ==8591==    by 0x447162: ActivateDevice (devices.c:530)
> > > ==8591==    by 0x4475D6: InitCoreDevices (devices.c:672)
> > > ==8591==    by 0x4449EE: main (main.c:254)
> > > 
> > 
> > for extra fun - this bug is present in pretty all server versions since 1.2
> > but didn't get exposed until the xalloc → malloc changes. I just took the
> > 1.7 sources and valgrind didn't complain. Changed xalloc to malloc, valgrind
> > gives out.
> > 
> > Any guesses to why xalloc hides this issue are appreciated.
> 
> It could be that you need to register xalloc as an allocation function
> with Valgrind. I know that it has some internal magic for standard
> allocators/freers.

xalloc was a macro to Xalloc, which was a simple function calling malloc. I
thought that should be transparent enough. 

Either way, might be worth doing a couple of valgrind checks on the server
now, who knows what else was covered up like this.

Cheers,
  Peter