[Freeassociation-devel] [PATCH] Memory corruption in timezone handling

[Robie Basak]

On Tue, Apr 03, 2012 at 10:44:08AM -0400, Wilfried Goesgens wrote:
> from the point of when to have a const pointer and when not, I realy
> think the whole tz loading & using solution is flawed.
> imho we should load that list into memory at start (so the application
> needs to call an libical-init which opens the configuration and loads it
> into memory)
> and work with const pointers all over the place later on.

This is what evolution tries to get libical to do currently, but it
fails in my case because of a special case in
icaltimezone_get_builtin_timezone:

    /* Check whether file exists, but is not mentioned in zone.tab.
       It means it's a deprecated timezone, but still available. */

> I tried to get a consistant scheme which functions need const pointers,
> and which don't but didn't get to a point where I had a situation that
> would work without casting off the const in some place.

I don't think it's just about a const pointer, but also an API guarantee
that a returned pointer is not invalidated in the future. I did think
about adjusting icalarray.c to make this the case, using linked lists
between segments instead of growing/moving the entire array, but it
seemed quite messy. It would be doable though (if hacky) because
icalarray.h does abstract access to the array suitably well.

> Your valgrind log exactly points out that its fundamentaly broken and
> needs to be fixed.
> Â 
> So, we probably need to align on a scheme to centraly load the timezone,
> and remove the implicit loading from all other (deeper) places.

I'm just worried about the special case above, but this does seem like a
good idea. It seems to me that evolution is trying to force this on load
to avoid this issue at the moment.

> This will also remove probable race conditions when working in threaded
> mode.
> This will have to break the ABI.

Agreed.

In the meantime, would you be willing to take this patch as a temporary
workaround, given that it's causing memory corruption in its current
state?

Since the problem is with the builtin_timezones array growing (and thus
moving), if I set the initial size of the array large enough, the
problem goes away. On my system, this stops evolution from crashing on
calendar startup. I imagine sizeof(icaltimezone) to be quite small, so I
don't think this will waste too much memory.


Index: libical-0.48/src/libical/icaltimezone.c
===================================================================
--- libical-0.48.orig/src/libical/icaltimezone.c	2011-12-13 17:08:18.000000000 +0000
+++ libical-0.48/src/libical/icaltimezone.c	2012-04-01 12:15:00.836064296 +0000
@@ -1656,7 +1656,7 @@
     icalerror_assert (builtin_timezones == NULL,
 		      "Parsing zones.tab file multiple times");
 
-    builtin_timezones = icalarray_new (sizeof (icaltimezone), 32);
+    builtin_timezones = icalarray_new (sizeof (icaltimezone), 1024);
 
 #ifndef USE_BUILTIN_TZDATA
     filename_len = strlen ((char *) icaltzutil_get_zone_directory()) + strlen (ZONES_TAB_SYSTEM_FILENAME)