632352 - Items with base64 encoded files cause dataloss, missing timezone components cause caldav error

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[PM]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.9.2.13) Gecko/20101207 Lightning/1.0b3pre Thunderbird/3.1.7

when a caldav parsing error occur, an error message is displayed in the console :
Erreur : response is not defined


Reproducible: Always

Steps to Reproduce:
1.an event with attached file is created with evolution on a davical server.
2. sample event:
BEGIN:VCALENDAR
CALSCALE:GREGORIAN
PRODID:-//Ximian//NONSGML Evolution Calendar//EN
VERSION:2.0
BEGIN:VTIMEZONE
TZID:/freeassociation.sourceforge.net/Tzfile/Europe/Paris
X-LIC-LOCATION:Europe/Paris
BEGIN:STANDARD
TZNAME:CET
DTSTART:19701030T030000
RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=10
TZOFFSETFROM:+0200
TZOFFSETTO:+0100
END:STANDARD
BEGIN:DAYLIGHT
TZNAME:CEST
DTSTART:19700327T020000
RRULE:FREQ=YEARLY;BYDAY=-1SU;BYMONTH=3
TZOFFSETFROM:+0100
TZOFFSETTO:+0200
END:DAYLIGHT
END:VTIMEZONE
BEGIN:VEVENT
UID:20110207T101844Z-4274-1000-1-0@pm-desktop
DTSTAMP:20110207T101844Z
DTSTART;TZID=/freeassociation.sourceforge.net/Tzfile/Europe/Paris:
 20110207T113000
DTEND;TZID=/freeassociation.sourceforge.net/Tzfile/Europe/Paris:
 20110207T120000
TRANSP:OPAQUE
SEQUENCE:2
SUMMARY:blankpdf
CLASS:PUBLIC
CREATED:20110207T101902Z
LAST-MODIFIED:20110207T101902Z
ATTACH;VALUE=BINARY;ENCODING=BASE64;
 X-EVOLUTION-CALDAV-ATTACHMENT-NAME=blankpdf.pdf:
 JVBERi0xLjQKJcOkw7zDtsOfCjIgMCBvYmoKPDwvTGVuZ3RoIDMgMCBSL0ZpbHRlci9GbGF0ZU
 RlY29kZT4+CnN0cmVhbQp4nDPQM1Qo5ypUMABCU0tTPRMFCxMjhaJUhXAthTyuQAUAbSUGxgpl
 bmRzdHJlYW0KZW5kb2JqCgozIDAgb2JqCjM4CmVuZG9iagoKNSAwIG9iago8PAo+PgplbmRvYm
 oKCjYgMCBvYmoKPDwvRm9udCA1IDAgUgovUHJvY1NldFsvUERGL1RleHRdCj4+CmVuZG9iagoK
 MSAwIG9iago8PC9UeXBlL1BhZ2UvUGFyZW50IDQgMCBSL1Jlc291cmNlcyA2IDAgUi9NZWRpYU
 JveFswIDAgNTk1IDg0Ml0vR3JvdXA8PC9TL1RyYW5zcGFyZW5jeS9DUy9EZXZpY2VSR0IvSSB0
 cnVlPj4vQ29udGVudHMgMiAwIFI+PgplbmRvYmoKCjQgMCBvYmoKPDwvVHlwZS9QYWdlcwovUm
 Vzb3VyY2VzIDYgMCBSCi9NZWRpYUJveFsgMCAwIDU5NSA4NDIgXQovS2lkc1sgMSAwIFIgXQov
 Q291bnQgMT4+CmVuZG9iagoKNyAwIG9iago8PC9UeXBlL0NhdGFsb2cvUGFnZXMgNCAwIFIKL0
 9wZW5BY3Rpb25bMSAwIFIgL1hZWiBudWxsIG51bGwgMF0KL0xhbmcoZnItRlIpCj4+CmVuZG9i
 agoKOCAwIG9iago8PC9DcmVhdG9yPEZFRkYwMDU3MDA3MjAwNjkwMDc0MDA2NTAwNzI+Ci9Qcm
 9kdWNlcjxGRUZGMDA0RjAwNzAwMDY1MDA2RTAwNEYwMDY2MDA2NjAwNjkwMDYzMDA2NTAwMkUw
 MDZGMDA3MjAwNjcwMDIwMDAzMzAwMkUwMDMyPgovQ3JlYXRpb25EYXRlKEQ6MjAxMTAyMDcxMT
 E4MDIrMDEnMDAnKT4+CmVuZG9iagoKeHJlZgowIDkKMDAwMDAwMDAwMCA2NTUzNSBmIAowMDAw
 MDAwMjIyIDAwMDAwIG4gCjAwMDAwMDAwMTkgMDAwMDAgbiAKMDAwMDAwMDEyOCAwMDAwMCBuIA
 owMDAwMDAwMzY0IDAwMDAwIG4gCjAwMDAwMDAxNDcgMDAwMDAgbiAKMDAwMDAwMDE2OSAwMDAw
 MCBuIAowMDAwMDAwNDYyIDAwMDAwIG4gCjAwMDAwMDA1NTggMDAwMDAgbiAKdHJhaWxlcgo8PC
 9TaXplIDkvUm9vdCA3IDAgUgovSW5mbyA4IDAgUgovSUQgWyA8MUJBNjY2ODhDNzY4ODhBN0U5
 NUEzMDRCMDM4MTlDN0Q+CjwxQkE2NjY4OEM3Njg4OEE3RTk1QTMwNEIwMzgxOUM3RD4gXQovRG
 9jQ2hlY2tzdW0gL0FBMzE1MjQ5NTJDNUQ1RERDRTcwRDQ3ODdDQTdGQkE4Cj4+CnN0YXJ0eHJl
 Zgo3NDQKJSVFT0YK
END:VEVENT
END:VCALENDAR
3. when using lightning to read this event, a parsing error occurs (not every time!) but not reported correctly in the console.
Actual Results:  
'Erreur : response is not defined' is displayed in the console. Event is not shown.


error message is produced in 'calendar/providers/caldav/calDavCalendar.js' file when an exception occur, function caldav_addTargetCalendarItem.

Comment 1

[PM]

Attachment: patch to display parsin error in the console

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

We need to look into this, confirmed per upcoming duplicate.

Comment 4

[Miquel van Smoorenburg]

I'm not quite sure this bug is exactly the same as 632981. Please have a look at 632981 as it contains a clearer description of what happens and an actual patch to fix the problem (well, a bandaid really).

Comment 5

[PM]

I also think that the bug 632981 is different.
bug 632981 seems to be a problem in timezone DTSTART and DTEND.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

The error is the same, I was thinking that when fixed with an operation rather than a bandaid, both issues should go away automatically. I'll make sure this is the case before uploading a patch.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Testcase ics

Strange stuff happening here. Under Lightning Trunk I don't get this error, but with the attached event, the item doesn't even show up.

With my instance of 1.0b2, the event shows, I don't get the response not defined error, but I do get a message that my calendar can't be used, because of "undefined".

I also tried the ics from the duplicate bug, this works for me without problems on an uncached calendar (aside from the mandatory timezone definition missing and a message regarding that being displayed). With a cached calendar I get an error in calStorageCalendar.js, which I have a small patch for (stores floating if the timezone component cannot be found). I will upload this patch as soon as the other issues are identified.

Comment 8

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

This patch solves both issues and also fixes bug 496699. The attachment data is no longer forced to be an uri, a missing timezone component doesnt break the storage calendar and I've also decided to remove the alarm service log messages since they just fill up the error console with unneeded data.

The main issue is of course also fixed, we were using an old variable name in the catch block.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Hmmm...this may sound a bit strange, but we might want to keep this bug! Right now libical can't handle the ATTACH property right and will kind of strip out the attach property's data, or at least fill it with nonsense.

The above patch will make events with attached binary data visible, so changing a such event will cause dataloss!

I'll see if I can get libical to do the right thing, but no promises!

Comment 10

[PM]

I tested the patch. 
When opening an event with attached binary data, 
an exception is always raised when calling makeURL form get uri (calAttachment.js file).
I added trace and it seems that makeURL is called with the content of attached binary data.

Then I did some tests parsing / serialization. 
/*
let rootComp=icsService.parseICS(str, null);
rootComp.serializeToICS();
*/
I found that the final content is different from the original.

I think there is a problem with the handling of binary data in libical.
There is a problem of memory corruption when processing binary data attachment.
When parsing a line (icalparser_add_line function - icalparser.c) a buffer is allocated (str)
and passed as a parameter to the function icalvalue_new_from_string
which is used in the function icalattach_new_from_data (icalparser.c).
In this function the pointer str is copied.
In return (icalparser_add_line function), icalmemory_free_buffer is executed on the buffer str.
The binary data are no longer valid.

I have made a patch witch correct this problem.
When applying this patch, we see that makeURL is called with the content of attached binary data.

As lightning dont support (yet) binaries attachments, they should be ignored.
I have made a second patch witch ignore binaries attachments. Applying this patch prevent from exception raising.

Comment 11

[PM]

Attachment: Patch for correct binaries attachments handling in libical

This patch corrects the problem of managing binary data in the library libical.

Comment 12

[PM]

Attachment: patch to ignore binary attachments

this patch does not add binaries attachments in calItemBase

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v2

Thanks PMARTINAK, your patch gave me the right hint! I've taken a look at the latest libical (0.46), they have also included the same fix. To stay closer to what libical does, I think we should update libical. I have filed bug 637064 for this. If we don't do this for the release, then we could take your fix, but formatted like in libical so its easier to upgrade later on.

Aside from that, I found out that the event dialog swallows attachments that don't use uris. With this patch we don't have to ignore binary attachments and don't cause dataloss. I've also made sure that the attach value is properly set.

Comment 15

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 515393 [details] [diff] [review]
Fix - v2

Philippe, since you have a good eye for what needs to be fixed, I would appreciate if you could take over the review for this bug.

Comment 16

[PM]

With patch v2, the opening event with attachments in the form of URL is working correctly. Attachments can be opened correctly.
In the case of attachments as binary, call 'makeURL' always generates an exception. 
Does the type of attachment can be tested before the call to 'makeURL' in calAttachment get uri? 
For binary attachments, calAttachment getParameter ("VALUE") return 'BINARY'. This parameter is not set for attachments in the form of URL. 

In the 'calendar-event-dialog.js',' attachment.rawData is used as an index into gAttachMap.
attachment.rawData may contain binary data size (very) important. Is this the best approach?

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #16)
> In the case of attachments as binary, call 'makeURL' always generates an
> exception. 
> Does the type of attachment can be tested before the call to 'makeURL' in
> calAttachment get uri? 
> For binary attachments, calAttachment getParameter ("VALUE") return 'BINARY'.
> This parameter is not set for attachments in the form of URL. 
Yes, this should be doable. On this occasion I've noticed that calAttachment doesn't actually set the BINARY parameter. I'll fix this in the next version.


> In the 'calendar-event-dialog.js',' attachment.rawData is used as an index into
> gAttachMap.
> attachment.rawData may contain binary data size (very) important. Is this the
> best approach?
No, its not the best approach, I agree. Since the binary data may be very large its probably not wise to make it the key for an object. I see no other ad hoch solution for this at the moment though. I guess an md5sum of the data could be calculated.

Thank you for looking at the code, your comments are very valuable!

Comment 18

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 515393 [details] [diff] [review]
Fix - v2

r- due to previous comments. New patch is coming up.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v3

This fixes the comments you've made. Does this look good? I needed to try/catch the URL anyway, it could be that an ATTACH property is specified that contains an invalid URL.

Comment 21

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v4

This fixes a few errors I found when creating a unit test (which is also included). There is still a problem with binary attachments, but I believe this will be fixed by libical 0.46. I think I will commit libical now, so we can find regressions early.

Comment 22

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v5

And yet another round that works without compile errors on the just-checked-in libical-0.4.6. Sorry for the vast number of patches, this should be it!

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 534897 [details] [diff] [review]
Fix - v5

Given Philippe has already given review comments on the previous patch, I'm going to mark this r+ in the sense of "r+ with that fixed".

Comment 24

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/cbc47df09441>
-> FIXED

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Backported to comm-miramar <http://hg.mozilla.org/releases/comm-miramar/rev/2ca68db65798>

Comment 26

[PM]

Attachment: patch thisCalendar notdefined in calDavRequestHandlers.js file

After some further testing, i found an error in calDavRequestHandlers.js file.
Under some conditions, an error is displayed in the console:
Avertissement : CalDAV: Get failed: CalDAV: Error: got status 200 fetching calendar data for davical, null
Looking at the code, some var are not defined.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 537539 [details] [diff] [review]
patch thisCalendar notdefined in calDavRequestHandlers.js file

Good catch, thanks for the patch!

Comment 28

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/05d369d9ecaf>
-> FIXED

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

Backported to comm-miramar <http://hg.mozilla.org/releases/comm-miramar/rev/f0a1a84e35ed>