Open Bug 692574 Opened 9 years ago Updated 4 years ago

RFC2231/5987 decoding should not tolerate missing single quotes

Categories

(Firefox :: File Handling, defect)

defect
Not set
normal

Tracking

()

REOPENED

People

(Reporter: julian.reschke, Assigned: julian.reschke)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

See 

  http://greenbytes.de/tech/tc2231/#attwithfn2231singleqmissing

only FF is tolerant here. We should ignore the param instead.
Blocks: 609667
Assignee: nobody → julian.reschke
Attached patch Proposed patch, incl test case (obsolete) — Splinter Review
depends on patch for 686429
Attachment #565797 - Flags: review?(bzbarsky)
Target Milestone: --- → mozilla10
This code seems to be pretty purposeful.  Have you checked the history to see when/why it was added?

Also why is the aCharset null-check ok to remove?
(In reply to Boris Zbarsky (:bz) from comment #2)
> This code seems to be pretty purposeful.  Have you checked the history to
> see when/why it was added?

Apparently it predates 2003/2004, when the code was moved over from the mail component. I can't find anything specific to this in Bugzilla.

> Also why is the aCharset null-check ok to remove?

It's not (unfortunately this is something we can't test from JS, right?). Will update the patch. (And thanks for checking!)
(In reply to Julian Reschke from comment #3)
> (In reply to Boris Zbarsky (:bz) from comment #2)
> > This code seems to be pretty purposeful.  Have you checked the history to
> > see when/why it was added?
> 
> Apparently it predates 2003/2004, when the code was moved over from the mail
> component. I can't find anything specific to this in Bugzilla.
> ...

As far as I can tell, this has been around in the initial checkin for bug 162765. I *assume* it was done in the spirit of being lenient, but as far as I can tell, it doesn't need to, and being lenient in only one implementation actually is bad for interop.
(restores the missing aCharset check)
Attachment #565797 - Attachment is obsolete: true
Attachment #565797 - Flags: review?(bzbarsky)
Attachment #566546 - Flags: review?(bzbarsky)
> As far as I can tell, this has been around in the initial checkin for bug 162765

Perfect, thanks.  The mailnews code that was removed in that checkin looked like this:

-  const char *s_quote1 = PL_strchr(value_start, 0x27);
-  const char *s_quote2 = (char *) (s_quote1 ? PL_strchr(s_quote1+1, 0x27) : NULL);
-  NS_ASSERTION(s_quote1 && s_quote2, "1.1 <rhp@netscape.com> 19 Mar 1999 12:00");
-  if (charset && s_quote1 > value_start && s_quote1 < value_end)
-    /* set *charset */
....
-  if (language && s_quote1 && sa_quote2 && s_quote2 > s_quote1+1 &&
-    s_quote2 < value_end)
-    /* set *language */
...
-  if (s_quote2 && s_quote2+1 < value_end)
-    /* set s, etc */

So it looks like it set *charset no matter what, but asserted if there wasn't a second quote and pressed on.  <sigh>.

Let's hope that mail doesn't depend on this behavior...

> unfortunately this is something we can't test from JS, right?

That's correct.
Comment on attachment 566546 [details] [diff] [review]
Proposed patch, incl test case

r=me
Attachment #566546 - Flags: review?(bzbarsky) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b87afed1d1c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 704989
Reopened because the change was backed out for now due to bug 704989.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Product: Core → Firefox
Target Milestone: mozilla10 → ---
You need to log in before you can comment on or make changes to this bug.