[PATCH] xkb: when copying the keymap, make sure the structs default to 0/NULL.

[Rene van Paassen]

On 07/02/2008, Daniel Stone <daniel at fooishbar.org> wrote:
> On Thu, Feb 07, 2008 at 04:03:07PM +1030, Peter Hutterer wrote:
> > Finally found the bug. Turns out that when copying keymaps, parts of
> > structs remain unintialised. this is a bad idea if you're trying to free
> > various pointers lateron (in my case dst->geom->sections->doodads).
>
> Hi,
> My general understanding is that you wouldn't try to free foo if sz_foo
> was 0.  Of course, this being XKB, there's a lot of superfluous crap
> that turns out to be unused, and the result makes no sense. \o/
>
> > +            dst->geom->shapes = NULL;
> >               dst->geom->num_shapes = 0;
>
> Please no tab damage. :)
>
> >                   tmp = xalloc(src->geom->num_sections *
> > sizeof(XkbSectionRec));
> >               if (!tmp)
> >                   return FALSE;
> > +            memset(tmp, 0, src->geom->num_sections *
> > sizeof(XkbSectionRec));
>
> Looks fine to me?
>
> > +                } else
> > +                    dsection->doodads = NULL;
>
> Please use:
> }
> else {
>     foo;
> }
>
> for consistency.
>
> >               if (dst->geom->sz_key_aliases && dst->geom->key_aliases) {
> >                   xfree(dst->geom->key_aliases);
> > -                dst->geom->key_aliases = NULL;
> >               }

are the xfree semantics different from the malloc/free semantics? free
should not trip over a NULL pointer. So we should NOT test for
dst->geom->key_aliases (etc.) to be non-NULL before freeing these
pointers.

free(NULL); is perfectly valid.

p.s. I learned this after a good portion of my code had been checked
for coverage. All these

if (pointer) {
   free(pointer);
}

paths simply complicate coverage analysis and are harder to read.


> If we should unconditionally free non-NULL pointers despite sz_foo (god
> this makes less and less sense every time I type it), then we should
> probably just test for key_aliases here, not sz_key_aliases.

IN that case unconditionally free ALL pointers! Unless I am mistaken
in interpreting xfree semantics.

>
> >               if (dst->geom->label_font) {
> >                   xfree(dst->geom->label_font);
> > -                dst->geom->label_font = NULL;
> >               }
> > +            dst->geom->label_font = NULL;
>
> This hunk has no effect.
>
> Otherwise looks good though, thanks for tracking this down.
>
> Cheers,
> Daniel

Greetings,
René