Closed Bug 875739 Opened 7 years ago Closed 4 years ago

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

Categories

(Calendar :: Internal Components, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: joerg.reinholz, Assigned: bv1578)

References

Details

(Keywords: hang)

Attachments

(4 files, 4 obsolete files)

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.
Severity: normal → critical
Component: Untriaged → General
Keywords: hang
Product: Thunderbird → Calendar
Version: 17 → unspecified
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.
Summary: Thunderbird 17.0.6 (Ubuntu 12.04 and 13.04, 64 Bit) + Ligthning 1.9 + Provider for Google-Calendar hanging on Mail with ICS-file → Freeze (hang) on .ics file which has two CN (common name) parameters set on the ATTENDEE property
Looks like a problem within libical backend because it seems to work when using the icaljs backend instead.
Component: General → Internal Components
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
Example of a event with duplicate x-param, impairing (among several others) TB17.0.8 with Lightning 1.9.1 / Win.
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.
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
Duplicate of this bug: 940963
Duplicate of this bug: 973772
Summary: Freeze (hang) on .ics file which has two CN (common name) parameters set on the ATTENDEE property → Freeze (hang) on .ics file which has two CN (common name) parameters set on the ATTENDEE/ORGANIZER property
Duplicate of this bug: 1073794
Duplicate of this bug: 1066247
Summary: Freeze (hang) on .ics file which has two CN (common name) parameters set on the ATTENDEE/ORGANIZER property → Freeze (hang) on .ics file which has two CN (common name) parameters or duplicated X-params set on the ATTENDEE/ORGANIZER property
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.
(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.
I have, so far, never seen non matching sequence numbers such as:
 X-RECEIVED-SEQUENCE=2;X-RECEIVED-SEQUENCE=0:mailto:address@domain.com

I have only seen:
 X-RECEIVED-SEQUENCE=0;X-RECEIVED-SEQUENCE=0:mailto:address@domain.com
or 
 X-RECEIVED-SEQUENCE=2;X-RECEIVED-SEQUENCE=2:mailto:address@domain.com


Is X-RECEIVED-SEQUENCE=0 the same as X-RECEIVED-SEQUENCE=0:mailto:address@domain.com?

If that is considered the same, then shouldn't to just read it or write it out as
 X-RECEIVED-SEQUENCE=0:mailto:address@domain.com
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.
Duplicate of this bug: 1106529
I came across this
https://bugzilla.mozilla.org/show_bug.cgi?id=534228

Can that patch be modified to also include X-RECEIVED-SEQUENCE
Duplicate of this bug: 1119761
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.
Duplicate of this bug: 813223
Duplicate of this bug: 1170508
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) ?
(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.
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 ?
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;
 }
}
Attached patch calAttendee.patch (obsolete) — Splinter Review
Attachment #8656698 - Flags: feedback?(philipp)
Attached patch calIteratorUtils.jsm.patch (obsolete) — Splinter Review
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 ...
Attachment #8656698 - Attachment is obsolete: true
Attachment #8656698 - Flags: feedback?(philipp)
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 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.
Attachment #8657072 - Flags: feedback+
Attached patch calIteratorUtils.jsm.patch (obsolete) — Splinter Review
Modified workaround to only use on loop.
Attachment #8657072 - Attachment is obsolete: true
Attached patch calIteratorUtils.jsm.patch (obsolete) — Splinter Review
Corrected indentation.
Attachment #8657188 - Attachment is obsolete: true
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
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".
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.
Duplicate of this bug: 1265701
Duplicate of this bug: 1272313
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:user.b@example.com
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:user.a@example.com
X-MOZ-GENERATION:2
END:VEVENT
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.
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.
Attachment #8657189 - Attachment is obsolete: true
Attachment #8758060 - Flags: review?(philipp)
test with attendees and organizer with double parameters.
Attachment #8758061 - Flags: review?(philipp)
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
Attachment #8758060 - Flags: review?(philipp) → review+
Attachment #8758061 - Flags: review?(philipp) → review+
Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/de9936da55acede438b8e11a2baab69246b9ef28
https://hg.mozilla.org/comm-central/rev/053758b8389e73c42a1bd309f9d99ca8402f5de3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 5.2
Assignee: nobody → bv1578
Duplicate of this bug: 1290822
Duplicate of this bug: 1370305
You need to log in before you can comment on or make changes to this bug.