GNOME Bugzilla – Bug 593019
"Etc/UTC" timezone + e_cal_get_timezone: g_assert() aborts program
Last modified: 2013-09-14 16:53:09 UTC
Created attachment 141648 [details] triggers syszone assert A SyncEvolution user reported that SyncEvolution aborted when applied to his calendar: libecal:ERROR:e-cal.c:4761:e_cal_get_timezone: assertion failed: (syszone) It turned out that he had a VEVENT with a TZID=Etc/UTC in his calendar, which wasn't handled properly by e_cal_get_timezone(). I have a patch which explains this in more detail. To reproduce the problem, try importing the attached calendar.ics into Evolution.
Created attachment 141649 [details] [review] fixes the handing of "UTC" TZIDs
As mentioned on IRC... Patch seems fine to me, but defering to a calendar maintainer for approval. Please use /* ... */ style comments instead of //.
Created attachment 141651 [details] [review] fixes the handing of "UTC" TZIDs, now with C-style comments
Chenthill, can you check the patch?
Hmm, I'm not sure of the RFC 2445, as the [1] says the TZID must not be used when times are in UTC. They probably mean the times which ends with a "Z" only. This is, sort of, on the edge, from my point of view. [1] http://tools.ietf.org/html/rfc2445#section-4.2.19
From a standards point of view, DTSTART;TZID=Etc/UTC:20090826T160000 is a normal time stamp relative to a custom VTIMEZONE. There's nothing wrong with it. From a practical point of view, UTC is "meant", so DTSTART:20090826T160000Z would be more natural. DTSTART;TZID=Etc/UTC:20090826T160000Z would be wrong, which is what section-4.2.19 is about.
(In reply to comment #6) > DTSTART;TZID=Etc/UTC:20090826T160000Z would be wrong, which is what > section-4.2.19 is about. Yup, that's the clear thing there. What I'm not sure about is whether > DTSTART;TZID=UTC:20090826T160000 is also good or bad thing to do, as it's an equivalent of > DTSTART:20090826T160000Z
(In reply to comment #7) > (In reply to comment #6) > > DTSTART;TZID=Etc/UTC:20090826T160000Z would be wrong, which is what > > section-4.2.19 is about. > > Yup, that's the clear thing there. What I'm not sure about is whether > > DTSTART;TZID=UTC:20090826T160000 > is also good or bad thing to do, as it's an equivalent of > > DTSTART:20090826T160000Z Getting rid of the TZID would be nice. But it would also make the patch more complicated - is that desirable?
> Getting rid of the TZID would be nice. But it would also make the patch more > complicated - is that desirable? I've looked into this some more. Getting rid of a TZID=*/UTC isn't possible in e_cal_get_timezone(), the function where the UTC "timezone" is detected. The function simply hasn't access to the VEVENT or VJOURNAL data at all. I also considered simply returning the original VTIMEZONE instead of a builtin, pseudo one. The drawback is that there is a non-zero risk that some broken peers send an event with TZID=UTC without a corresponding VTIMEZONE; falling back to our own definition handles that. I therefore think that the suggested way of dealing with such a timezone (returning a VTIMEZONE which has the same semantic as the Z suffix) is still the best solution.
It wouldn be possible to change the tzid in that function. One question I have is, when exporting the calendar as .ics file from evolution is the tzid correctly exported ? does it get exported as Etc/UTC instead of just UTC ?
(In reply to comment #10) > One question I have > is, when exporting the calendar as .ics file from evolution is the tzid > correctly exported ? does it get exported as Etc/UTC instead of just UTC ? You are on to something here. No, it wasn't. However, that wasn't due to the code in e_cal_get_timezone(). e_cal_check_timezones() does the same "Etc/UTC" -> "UTC" mapping inside e_cal_match_timezone() and then imports the event into the calendar with TZID=UTC. Displaying the event works fine (I had tested that) but exporting it creates an event with DTSTART;TZID=UTC:20090825T160000 and no corresponding VTIMEZONE definition. Not good. My suggestion is to disable the mapping to "UTC" in e_cal_match_timezone() because e_cal_check_timezones() is not able to handle this correctly. I'll attach a patch. This makes Evolution import the example with TZID=Etc/UTC preserved unchanged, which then can be exported like that again (tested). The e_cal_get_timezone() patch then becomes obsolete. However, the assert should be removed (part of the patch). e_cal_match_timezone() could be changed so that it actually turns all time values into fixed UTC (Z suffix), but even after thinking about it a bit I don't see an easy way to do it. It has to be done correctly for all kinds of properties; some of them look complicated (EXDATE with list of time values!). I don't have time for such a patch and/or wouldn't be able to ensure that it really works in all cases.
Created attachment 143331 [details] [review] avoid problematic mapping to "UTC", remove asserts
Comment on attachment 143331 [details] [review] avoid problematic mapping to "UTC", remove asserts The patch looks good. Please commit the same to master immediately and stable after the hard code freeze ends next week. I don see any need to change the tzid though since it can be anything, just in this case its UTC.
Patch committed in master (2.29.1+) http://git.gnome.org/cgit/evolution-data-server/commit/?id=ccea859f88844c0cc3a94857e47eade6b1257cba @Patrick, please commit the patch in gnome-2-28 branch, change the patch statue in bug and close the bug, TIA
(In reply to comment #14) > Patch committed in master (2.29.1+) > http://git.gnome.org/cgit/evolution-data-server/commit/?id=ccea859f88844c0cc3a94857e47eade6b1257cba > > @Patrick, please commit the patch in gnome-2-28 branch, change the patch statue > in bug and close the bug, TIA Done, commit d90c3992e956c17e5dc6e34844529c827d35c749. I didn't commit it there earlier because Chenthil said saomething about a freeze.