GNOME Bugzilla – Bug 762985
segfaults calling icaltime_normalize() on null value
Last modified: 2017-04-17 18:20:40 UTC
That's a new issue on 3.19 with that code https://git.gnome.org/browse/gnome-calendar/tree/src/gcal-event.c#n219 " /* Setup end date */ e_cal_component_get_dtend (component, &end); date = icaltime_normalize (*end.value);" or if there are event without a dtend then end.value = null and the icaltime_normalize is an invalid read and leads to a segfault
Created attachment 323244 [details] [review] gcal-event: If there is no dtend field, set the end to the same as start RFC 2445 says 'A calendar entry with a "DTSTART" property but no "DTEND" property does not take up any time. It is intended to represent an event that is associated with a given calendar date and time of day, such as an anniversary.' It is legal to have no 'DTEND'. In this case we take the start time, so that the event is marked as an all day event. This was the behaviour up to 3.18, see: https://git.gnome.org/browse/gnome-calendar/tree/src/e-cal-data-model.c?h=gnome-3-18#n51
Created attachment 323246 [details] [review] (didn't need the .h change)
Review of attachment 323246 [details] [review]: Still some work to do. ::: src/gcal-event.c @@ +224,3 @@ e_cal_component_get_dtend (component, &end); + if (!end.value || icaltime_is_null_time (*end.value)) + end = start; Could you please make it end = start + 1 day?
Created attachment 323339 [details] [review] event: fix crash when no end date is given In this commit, we fixed a crash report that occurred when an event had no end date. Events with no end dates are all-day, 1-day length events. But, when an event with no end date set was synchronized with calendar (happened mostly with birthdays), it caused crashes, because the function responsible for getting the end date received NULL. To correct this, we considered that end = start + 1day when no end date is set. With this, the mistake was corrected. Another thing that was correct along with this were two memory leaks, caused by the allocation of both end and start date that were not been freed.
Review of attachment 323339 [details] [review]: Almost there. Thanks for fixing the leaks. ::: src/gcal-event.c @@ +219,3 @@ date = icaltime_normalize (*end.value); + + if(!end.value) This should come before icaltime_normalize(...) otherwise it'll still crash.
Created attachment 323340 [details] [review] event: fix crash when no end date is given In this commit, we fixed a crash report that occurred when an event had no end date. Events with no end dates are all-day, 1-day length events. But, when an event with no end date set was synchronized with calendar (happened mostly with birthdays), it caused crashes, because the function responsible for getting the end date received NULL. To correct this, we considered that end = start + 1day when no end date is set. With this, the mistake was corrected. Another thing that was correct along with this were two memory leaks, caused by the allocation of both end and start date that were not been freed.
Thanks for working on this issue. Attachment 323340 [details] pushed as e48c585 - event: fix crash when no end date is given
Created attachment 323383 [details] [review] event: If no end date is given, make the event be all_day explicitly Previously we were synthesising an end date of "start + 1 day", but this isn't necessary. RFC 2445 tells us that no end date already means an all day event. Check this case directly. --- The previous commit has an error - if end = start (the case for this bug) then this is a double free. I don't understand the "+ 1 day" thing. The RFC says that you don't need to make up an end date in this case as a missing DTEND already means all day. So this patch sets it directly. AFAICS the result is right. But if you don't like the change then please at least fix the double free - that's a crash.
Review of attachment 323383 [details] [review]: You're right, my bad. I find it funny how it didn't crash in my end after the previous patch, it's an obvious mistake (which I should've spotted earlier). ::: src/gcal-event.c @@ +262,3 @@ + g_clear_pointer (&zone_start, g_time_zone_unref); + if (zone_end) + g_clear_pointer (&zone_end, g_time_zone_unref); No need to check if(zone_...) here. g_clear_pointer() does that for us.
(In reply to Iain Lane from comment #8)> > I don't understand the "+ 1 day" thing. The RFC says that you don't need to > make up an end date in this case as a missing DTEND already means all day. So > this patch sets it directly. AFAICS the result is right. In Calendar the correct way to set a 1-day all day event is to have it span from (day 00:00) to (day+1 00:00). Please, respect this design choice.
(In reply to Georges Basile Stavracas Neto from comment #10) > (In reply to Iain Lane from comment #8)> > > I don't understand the "+ 1 day" thing. The RFC says that you don't need to > > make up an end date in this case as a missing DTEND already means all day. So > > this patch sets it directly. AFAICS the result is right. > > In Calendar the correct way to set a 1-day all day event is to have it span > from (day 00:00) to (day+1 00:00). Please, respect this design choice. I checked the result in the application. It seems that setting "self->all_day" does the right thing already (and is right per the RFC). But it's up to you to do what you want - that's not the really important part. (In reply to Georges Basile Stavracas Neto from comment #9) > Review of attachment 323383 [details] [review] [review]: > > You're right, my bad. I find it funny how it didn't crash in my end after > the previous patch, it's an obvious mistake (which I should've spotted > earlier). > > ::: src/gcal-event.c > @@ +262,3 @@ > + g_clear_pointer (&zone_start, g_time_zone_unref); > + if (zone_end) > + g_clear_pointer (&zone_end, g_time_zone_unref); > > No need to check if(zone_...) here. g_clear_pointer() does that for us. True.
Could you please update your patch? Besides the if(zone_...) thing, it looks good.
Created attachment 323407 [details] [review] event: If no end date is given, make the event be all_day explicitly Previously we were synthesising an end date of "start + 1 day", but this isn't necessary. RFC 2445 tells us that no end date already means an all day event. Check this case directly. -- I can push this too if you want.
Review of attachment 323407 [details] [review]: LGTM.
Attachment 323407 [details] pushed as ace8732 - event: If no end date is given, make the event be all_day explicitly