875739 - Freeze (hang) on .ics file which has two CN (common name) parameters or duplicated X-params set on the ATTENDEE/ORGANIZER property

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Jörg Reinholz]

Attachment: 5EC6E2CF-A4A3-46AA-B594-E5DBEFB9EDCB-301-00000D51231D0FD2.ics

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.22 (KHTML, like Gecko) Ubuntu Chromium/25.0.1364.160 Chrome/25.0.1364.160 Safari/537.22

Steps to reproduce:

I has tryed to open a mail with a ics-file. Exactly i have startet thunderbird and thunderbird has tryed to open the mail an given the preview.




Actual results:

Thunderbird hanges and use 100% of one (1 from 4) cpu-kernels. I have waited one hour and then i have shot up Thunderbird, then i has deleted this mail(s) via Webmail and the reopened Fireforks works now fine.

But Orage & Google Calendar can import the file properly. The ICS File (anonymised & appended) is created with "Marketcircle Inc.//Daylite 4.0".

This bug is tested  with 3 ICS-files from the same sender on 2 similar installations. (ubuntu 12.04, ubuntu 13.04)


Expected results:

Thunderbird shold not hanging on if there is a problem with a ics-file.

Comment 1

[Stefan Sitter [:ssitter]]

Confirmed using Lightning 2.7a1 with Thunderbird 25.0a1. Hang is caused by the following attendee entry:

> ATTENDEE;ROLE=CHAIR;CN=DELETED;CUTYPE=INDIVIDUAL;
>  PARTSTAT=ACCEPTED;CN=DELETED:mailto:
>  DELETED@DELETED

It seems the problems is that there are two CN (common name) parameters set on the entry. The calendar can be loaded fine after removing either the first or the second CN parameter.

According to the iCalendar specification <http://tools.ietf.org/html/rfc5545#page-108> the event is invalid because CN is only allowed once:

> attendee   = "ATTENDEE" attparam ":" cal-address CRLF
> 
>        attparam   = *(
>                   ;
>                   ; The following are OPTIONAL,
>                   ; but MUST NOT occur more than once.
>                   ;
>                   (";" cutypeparam) / (";" memberparam) /
>                   (";" roleparam) / (";" partstatparam) /
>                   (";" rsvpparam) / (";" deltoparam) /
>                   (";" delfromparam) / (";" sentbyparam) /
>                   (";" cnparam) / (";" dirparam) /
>                   (";" languageparam) /

Of course Lightning should not break on such an invalid event.

Comment 2

[Stefan Sitter [:ssitter]]

Looks like a problem within libical backend because it seems to work when using the icaljs backend instead.

Comment 3

[jens.poenninghaus]

Hi,

we are suffering from a similar issue taking down a whole group of users.
In our case, it seems to be triggered by duplicate X-RECEIVED-DTSTAMP and X-RECEIVED-SQEQUENCE parameters within an attendee property.

http://tools.ietf.org/html/rfc5545#section-3.2
> Applications MUST ignore x-param and iana-param values they don't
>   recognize.
I'd by happy to see an exception rather than a freeze that effectively bans ordinary users from using thunderbird mail, too.

Cheers
Jens

Comment 4

[jens.poenninghaus]

Attachment: duplicate X-param on attendee blasting lightning

Example of a event with duplicate x-param, impairing (among several others) TB17.0.8 with Lightning 1.9.1 / Win.

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hey Jens, I'm sorry but its probably a little late in the game for Lightning 2.6. I unfortunately don't have time to fix this right now, but if an easy fix emerges we could consider creating a 2.6.1 build some time.

I do hope that for the next major release (i.e Lightning 2.3), we will no longer need libical, obsoleting this bug.

Comment 6

[jens.poenninghaus]

Hi Philipp,
 
thanks for the quick feedback. My timing is indeed suboptimal but I have to take them as they come ;-). So, I'll try to identify the application creating those events and meanwhile we can probably live with the bug, as it is probably of low frequency, reasonably easy to spot once you know what to look for and trivial to fix.

Cheers
Jens

Comment 11

[Karl Rossing]

I ran into the issue of duplicated X-RECEIVED-SEQUENCE. see https://bugzilla.mozilla.org/show_bug.cgi?id=1066247 which was marked as a duplicate of this bug.

Comment 12

[石庭豐 (Seak, Teng-Fong)]

(In reply to Stefan Sitter from comment #1)

> According to the iCalendar specification
> <http://tools.ietf.org/html/rfc5545#page-108> the event is invalid because
> CN is only allowed once:
> 
> > attendee   = "ATTENDEE" attparam ":" cal-address CRLF
> > 
> >        attparam   = *(
> >                   ;
> >                   ; The following are OPTIONAL,
> >                   ; but MUST NOT occur more than once.
> > [deleted]
> 
> Of course Lightning should not break on such an invalid event.

But we have to define TB behaviour facing such wrong data.  Does RFC 5545 define the recommended behaviour for an application when such event is received?  I have not read through the whole RFC (sorry :p) but I have three ideas:-
1. The whole event is totally rejected and user is informed (by a dialog) that such event has not been imported because of syntax error.

2. The first occurrence of every duplicate option is used while the others are ignored.  User is informed (by a dialog) telling him there is syntax error in the event.

3. The last occurrence of every duplicate option is used while the others are ignored.  User is informed (by a dialog) telling him there is syntax error in the event.

Personally, I would opt for #1 because RFC 2119 states that:
MUST NOT
 This phrase, or the phrase "SHALL NOT", mean that the definition is an *absolute* prohibition of the specification.

Comment 13

[Karl Rossing]

I have, so far, never seen non matching sequence numbers such as:
 X-RECEIVED-SEQUENCE=2;X-RECEIVED-SEQUENCE=0:mailto:[e-mail]

I have only seen:
 X-RECEIVED-SEQUENCE=0;X-RECEIVED-SEQUENCE=0:mailto:[e-mail]
or 
 X-RECEIVED-SEQUENCE=2;X-RECEIVED-SEQUENCE=2:mailto:[e-mail]


Is X-RECEIVED-SEQUENCE=0 the same as X-RECEIVED-SEQUENCE=0:mailto:[e-mail]?

If that is considered the same, then shouldn't to just read it or write it out as
 X-RECEIVED-SEQUENCE=0:mailto:[e-mail]

Comment 14

[石庭豐 (Seak, Teng-Fong)]

About your X-Params problem, let's move the discussion back to your original bug first, then we'll write the result/finding back to this bug.

Comment 16

[Karl Rossing]

I came across this
https://bugzilla.mozilla.org/show_bug.cgi?id=534228

Can that patch be modified to also include X-RECEIVED-SEQUENCE

Comment 18

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This bug is more about figuring out why it hangs and fixing that (probably in libical). From a brief look I don't think we can remove all the received sequence attributes, but that is something that should be figured out in bug 534228.

Comment 21

[diesmo]

Hello,

Same thing for us with lightning 4.0.2 on Windows 7.

Event causing the problem is also  duplicate X-RECEIVED-DTSTAMP and X-RECEIVED-SQEQUENCE parameters within an attendee property.

The problem seems to be in calbase.xpt, method getNextParameterName looping forever (loops on X-RECEIVED-DTSTAMP) while parsing the ical String.

Good news is with the icaljs property set to true, the event is correctly handled, no freeze:
user_pref("calendar.icaljs", true);

(or through the advanced configuration editor, search for ical, then double-click to set it to true)

Might this be an acceptable workaround (ie is it safe to enable this regarding implementation and performance) ?

Comment 22

[Stefan Sitter [:ssitter]]

(In reply to diesmo from comment #21)

I think biggest problem with the experimental icaljs library is currently its incorrect handling of recurring events, its incorrect handling of foreign timezones and the performance problems. If you don't mind those problems than you are welcome to test icaljs library.

Comment 23

[diesmo]

Ok, thanks for the advice. Not so good news then :).

To give additional details on the "getNextParameterName loops", if i try to import an event with

ATTENDEE;CN=Agendatest User1;PARTSTAT=DECLINED;ROLE=REQ-PARTICI
 PANT;SCHEDULE-STATUS=2.0;X-RECEIVED-DTSTAMP=20150811T093258Z;X
 -A=1;X-RECEIVED-SEQUENCE=0;X-B=3;X-RECEIVED-DTSTAMP=20150811T0
 93258Z;X-RECEIVED-SEQUENCE=0:mailto:user1.agendatest@example.o
 rg

in calendar-js/calAttende.js i modify the code to avoid the freeze and get debug information (line 89):

/*
        for each (let [name, value] in cal.ical.paramIterator(icalatt)) {
		Components.utils.reportError(name + '=' + value);
            if (!promotedProps[name]) {
                this.setProperty(name, value);
            }
        }*/
		let i=0;
		for (let paramName = icalatt.getFirstParameterName();
                     paramName;
                     paramName = icalatt.getNextParameterName()) {
			paramValue = icalatt.getParameter(paramName);
			if (!promotedProps[paramName]) {
                this.setProperty(paramName, paramValue);
            }
			Components.utils.reportError(paramName + '=' + paramValue);
			i++;
			if(i > 30) break;
		}


I get this in the console :

CN=Agendatest User1
PARTSTAT=DECLINED
ROLE=REQ-PARTICIPANT
SCHEDULE-STATUS=2.0
X-RECEIVED-DTSTAMP=20150811T093258Z
X-A=1
X-RECEIVED-SEQUENCE=0
X-B=3
X-RECEIVED-DTSTAMP=20150811T093258Z
X-A=1
X-RECEIVED-SEQUENCE=0
X-B=3
X-RECEIVED-DTSTAMP=20150811T093258Z
etc.

I hope this gives some clue on what is happening ?

Comment 24

[diesmo]

Also,

In calendar-js/calAttendee.js, if i get all the propertynames, then ask for values, i does not loop :

let icalParamNames = [];
for(let name in cal.ical.paramIterator(icalatt)) {
 icalParamNames.push(name);
}
for each (let name in icalParamNames) {
  let value = icalatt.getParameter(name);
  if (!promotedProps[name]) {
     this.setProperty(name, value);
  }
}

--> Would that be a good workaround ? 

Original code loops :
for each (let [name, value] in cal.ical.paramIterator(icalatt)) {
  if (!promotedProps[name]) {
    this.setProperty(name, value);
  }
}

And this code loops aswell :
let i=0;
for(let name in cal.ical.paramIterator(icalatt)) {
 let value = icalatt.getParameter(name);
 Components.utils.reportError(name + '=' + value);
 if (!promotedProps[name]) {
   this.setProperty(name, value);
 }
 i++;
 if (i > 30){
  Components.utils.reportError("Loop, i break!");
  break;
 }
}

Comment 25

[diesmo]

Attachment: calAttendee.patch

Comment 26

[diesmo]

Attachment: calIteratorUtils.jsm.patch

This patch gives a workaround for paramIterator that seems to loop when it contains duplicate keys.

The idea is to only iterate for property names (not values), and only then ask for each value.

The patch modifies modules/calIteratorUtils.jsm instead of calendar-js/calAttendee.js, becaus the problem is more in the paramIterator itself.

This is only a workaround, as the fix would be to modify libical ...

Comment 27

[diesmo]

I changed the proposed workaround to patch calIteratorUtils.jsm, but it's the same idea.

I don't think the code modification is harmful, it might be a little bit slower, though.

I tested on my event and the event provided in this bug :
 - create a local calendar
 - import the event attached to this bug : without the patch thunderbird hangs, with the patch it does not.

Any feedback on the workaround is appreciated.

Comment 28

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8657072 [details] [diff] [review]
calIteratorUtils.jsm.patch

Review of attachment 8657072 [details] [diff] [review]:
-----------------------------------------------------------------

I think you can simplify it a bit, using only one loop. Just add the parameter names to a new Set(), and check in each run if the param name was already added to the set. This shouldn't have any noticable performance implications.

Generally I think this is worth a shot though, especially since getParameter will not return different values for multiple parameters.

To admit this patch to Lightning, it would be great if you could create some unit tests. See this guide on how to get started:
https://developer.mozilla.org/en-US/docs/Simple_Thunderbird_build

If you have any questions, feel free to ask here or on irc.mozilla.org #calendar. My nickname is Fallen.

Comment 29

[diesmo]

Attachment: calIteratorUtils.jsm.patch

Modified workaround to only use on loop.

Comment 30

[diesmo]

Attachment: calIteratorUtils.jsm.patch

Corrected indentation.

Comment 31

[diesmo]

Ok, i modified the patch with your recommendations, thank you for that.

The link you provide is about building Thunderbird, is this link you wanted to point ?
or this one :
https://developer.mozilla.org/en-US/Add-ons/SDK/Tutorials/Unit_testing

Comment 32

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Lightning is built together with Thunderbird, therefore the link is correct. You'll have to set up a development environment to run the unit tests. We don't use the Add-ons SDK, but instead have a traditional XUL addon. You can find out more about the unit tests at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests and see other examples at https://dxr.mozilla.org/comm-central/source/calendar/test/unit . Once you've completed the build, you can run tests for example with "./mozilla/mach xpcshell-test test_alarm.js".

Comment 33

[diesmo]

I never built Thunderbird from scratch, and i'm working on another bug (lightning with wcap not detecting session timeouts).

I'll try to install and build after i'm done with the other bug.

Comment 36

[ldubois]

For the records, there are some reproduction steps to get this problem with a regular usage of TB 45.1 and Lightning 4.7(reproduced with Oracle Communication Convergence)

1. UserA invites userB to an event
2. UserB accepts
3. UserA receives an email with userB acceptation. UserA doesn't synchronize and clicks to the update button
4. UserB receives a notification email from userA
5. UserA clicks again on the update button, which is still in the blue bar
6. UserB receives another notification email. Then userA and userB Thunderbird hangs as described above

Now as long as this event is the calendars, userA and userB can't use TB anymore (it starts then freeze 3 sec later).

Related ICS, with 2 X-RECEIVED-DTSTAMP and X-RECEIVED-SEQUENCE

BEGIN:VEVENT
UID:dbf4eb98-0aa1-4f82-9cb1-22fa63837737
DTSTAMP:20160517T151651Z
SUMMARY:muli
DTSTART;TZID=Europe/Paris:20160527T100000
DTEND;TZID=Europe/Paris:20160527T110000
CREATED:20160517T151341Z
LAST-MODIFIED:20160517T151649Z
ORGANIZER;PARTSTAT=ACCEPTED;ROLE=CHAIR;RSVP=TRUE:mailto:[e-mail]
TRANSP:OPAQUE
ATTENDEE;CN=A User;PARTSTAT=ACCEPTED;ROLE=REQ-PARTICIPANT;RSVP=
 TRUE;SCHEDULE-STATUS=1.2;X-RECEIVED-DTSTAMP=20160517T151527Z;X-RECEIVED-
 DTSTAMP=20160517T151527Z;X-RECEIVED-SEQUENCE=0;X-RECEIVED-SEQUENCE=0:mai
 lto:[e-mail]
X-MOZ-GENERATION:2
END:VEVENT

Comment 37

[[:MakeMyDay]]

Just for the records, regarding the X-RECEIVED parameters/properties, there's already bug 534228 to remove them. However, the patch there is probably bitrotted.

Comment 38

[Decathlon]

Attachment: patch - v1 (by diesmo)

I wrote to diesmo who currently has no time to complete the patch so, with his permission, I try to complete the work.

I've adapted the patch to the changes caused by Bug 1221634 and added a test. Philipp, let me know if the test should be moved in a different place or in a separated file.

Comment 39

[Decathlon]

Attachment: test for patch - v1

test with attendees and organizer with double parameters.

Comment 40

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8758060 [details] [diff] [review]
patch - v1 (by diesmo)

Review of attachment 8758060 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for picking this up. Working around it in this way is better than freezing, lets do it! r=philipp

Comment 41

[Decathlon]

Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/de9936da55acede438b8e11a2baab69246b9ef28
https://hg.mozilla.org/comm-central/rev/053758b8389e73c42a1bd309f9d99ca8402f5de3