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

[Peter Hutterer]

Daniel Stone wrote:
> Hi,
> My general understanding is that you wouldn't try to free foo if sz_foo
> was 0.  
[...]
 > this being XKB,

one would think so. but then again the latter statement voids any 
assumption of sanity in the former.

> 
>> +            dst->geom->shapes = NULL;
>>               dst->geom->num_shapes = 0;
> 
> Please no tab damage. :)

I blame thunderbird or whatever, the actual patch looks good.

>>               if (dst->geom->sz_key_aliases && dst->geom->key_aliases) {
>>                   xfree(dst->geom->key_aliases);
>> -                dst->geom->key_aliases = NULL;
>>               }
> 
> 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.

XkbFreeGeomDoodads was what hit me. I guess there's more of them around. 
  They breed like rabbits.

cleaned-up patch attached. Thanks for the review.

 From 6079e5b56dc2e1338580044401511f83a4f2ddc5 Mon Sep 17 00:00:00 2001
From: Peter Hutterer <peter at cs.unisa.edu.au>
Date: Thu, 7 Feb 2008 15:48:04 +1030
Subject: [PATCH] xkb: when copying the keymap, make sure the structs 
default to 0/NULL.

It actually does help if a pointer is NULL rather than pointing to nirvana
when you're trying to free it lateron. Who would have thought?
---
  xkb/xkbUtils.c |   26 ++++++++++++++++++++------
  1 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/xkb/xkbUtils.c b/xkb/xkbUtils.c
index 68ecb32..148f9c9 100644
--- a/xkb/xkbUtils.c
+++ b/xkb/xkbUtils.c
@@ -1733,9 +1733,8 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, Bool 
sendNotifies)
          else {
              if (dst->geom->sz_shapes) {
                  xfree(dst->geom->shapes);
-                dst->geom->shapes = NULL;
              }
-
+            dst->geom->shapes = NULL;
              dst->geom->num_shapes = 0;
              dst->geom->sz_shapes = 0;
          }
@@ -1752,11 +1751,15 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, 
Bool sendNotifies)
                       j++, drow++) {
                      if (drow->num_keys)
                          xfree(drow->keys);
+
+                    drow->keys = NULL;
                  }

                  if (dsection->num_rows)
                      xfree(dsection->rows);

+                dsection->rows = NULL;
+
                  /* cut and waste from geom/doodad below. */
                  for (j = 0, ddoodad = dsection->doodads;
                       j < dsection->num_doodads;
@@ -1781,9 +1784,12 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, 
Bool sendNotifies)

                  if (dsection->num_doodads)
                      xfree(dsection->doodads);
+
+                dsection->doodads = NULL;
              }

              dst->geom->num_sections = 0;
+            dst->geom->sections = NULL;
          }

          if (src->geom->num_sections) {
@@ -1795,6 +1801,7 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, Bool 
sendNotifies)
                  tmp = xalloc(src->geom->num_sections * 
sizeof(XkbSectionRec));
              if (!tmp)
                  return FALSE;
+            memset(tmp, 0, src->geom->num_sections * 
sizeof(XkbSectionRec));
              dst->geom->sections = tmp;
              dst->geom->num_sections = src->geom->num_sections;

@@ -1809,6 +1816,8 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, Bool 
sendNotifies)
                          return FALSE;
                      dsection->rows = tmp;
                  }
+                dsection->num_rows = ssection->num_rows;
+
                  for (j = 0, srow = ssection->rows, drow = dsection->rows;
                       j < ssection->num_rows;
                       j++, srow++, drow++) {
@@ -1829,7 +1838,10 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, 
Bool sendNotifies)
                      if (!tmp)
                          return FALSE;
                      dsection->doodads = tmp;
+                } else {
+                    dsection->doodads = NULL;
                  }
+
                  for (k = 0,
                        sdoodad = ssection->doodads,
                        ddoodad = dsection->doodads;
@@ -1857,9 +1869,9 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, Bool 
sendNotifies)
          else {
              if (dst->geom->sz_sections) {
                  xfree(dst->geom->sections);
-                dst->geom->sections = NULL;
              }

+            dst->geom->sections = NULL;
              dst->geom->num_sections = 0;
              dst->geom->sz_sections = 0;
          }
@@ -1888,6 +1900,8 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, Bool 
sendNotifies)
                      }
                  }
              }
+            dst->geom->num_doodads = 0;
+            dst->geom->doodads = NULL;
          }

          if (src->geom->num_doodads) {
@@ -1900,7 +1914,7 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, Bool 
sendNotifies)
                                sizeof(XkbDoodadRec));
              if (!tmp)
                  return FALSE;
-            bzero(tmp, src->geom->num_doodads * sizeof(XkbDoodadRec));
+            memset(tmp, 0, src->geom->num_doodads * sizeof(XkbDoodadRec));
              dst->geom->doodads = tmp;

              dst->geom->sz_doodads = src->geom->num_doodads;
@@ -1929,9 +1943,9 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, Bool 
sendNotifies)
          else {
              if (dst->geom->sz_doodads) {
                  xfree(dst->geom->doodads);
-                dst->geom->doodads = NULL;
              }

+            dst->geom->doodads = NULL;
              dst->geom->num_doodads = 0;
              dst->geom->sz_doodads = 0;
          }
@@ -1961,8 +1975,8 @@ XkbCopyKeymap(XkbDescPtr src, XkbDescPtr dst, Bool 
sendNotifies)
          else {
              if (dst->geom->sz_key_aliases && dst->geom->key_aliases) {
                  xfree(dst->geom->key_aliases);
-                dst->geom->key_aliases = NULL;
              }
+            dst->geom->key_aliases = NULL;
              dst->geom->num_key_aliases = 0;
              dst->geom->sz_key_aliases = 0;
          }
-- 
1.5.3