588389 - content-disposition: escaped characters in quoted-string give funny results

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Julian Reschke]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729)

Given this C-D header field:

  Content-Disposition: attachment; filename="f\oo.html"

the expected filename would be

  foo.html

(in RFC 2616 quoted string, "\" is an escape character, and "\o" just stands for "o")

but the result is

  f_oo.html

(which maybe caused by something replacing "funny" characters *before* unescaping backslashes)

Reproducible: Always

Steps to Reproduce:
1. Run test at http://greenbytes.de/tech/tc2231/#attwithasciifnescapedchar

Actual Results:  
suggested file named is "f_oo.html"

Expected Results:  
suggested file should be "foo.html"

Comment 1

[Julian Reschke]

Attachment: test case for unescaping quoted-string

Comment 2

[Julian Reschke]

Attachment: do backslash unescaping then the parameter uses quoted-string syntax

Comment 3

[Reed Loden [:reed]]

Not sure if this should be File Handling or Networking...

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 473519 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

I'd prefer ++c to c+=1, but more importantly the OOM check needs to come before you start messing with the pointer returned from ToNewCString.

r=me with that fixed.

Comment 5

[Julian Reschke]

Attachment: do backslash unescaping then the parameter uses quoted-string syntax

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 481228 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

r=me

Comment 7

[Benjamin Smedberg]

What's the risk and reward here? Offhand it doesn't look like this is worth taking now.

Comment 8

[Benjamin Smedberg]

Comment on attachment 481228 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

Clearing nomination, please renominate when my question is addressed.

Comment 9

[Julian Reschke]

(In reply to comment #7)
> What's the risk and reward here? Offhand it doesn't look like this is worth
> taking now.

It's like with most conformance bug fixes; a risk may be there, but it's hard to predict. The reward is that messages do not get misinterpreted when using escapes.

That being said; this has been broken for so long that it's definitively not urgent enough to get into FF4, agreed.

Comment 10

[(no longer active)]

This patch doesn't apply cleanly on trunk any more.

Comment 11

[Julian Reschke]

Probably as it is in conflict with the change for 588781; will check.

Comment 12

[Julian Reschke]

Attachment: do backslash unescaping then the parameter uses quoted-string syntax

this includes a stand-alone JS test file; maybe it should be integrated into Jason's test_MIME_params.js?

Comment 13

[Julian Reschke]

(In reply to comment #7)
> What's the risk and reward here? Offhand it doesn't look like this is worth
> taking now.

Benjamin, this was pre-FF4. Is it ok to go ahead now it's out of the door?

Comment 14

[Benjamin Smedberg]

Yes, this can land.

Comment 15

[Christopher Blizzard (:blizzard)]

Ehsan says this has landed on cedar, on its way to mozilla-central.

Comment 16

[Boris Zbarsky [:bzbarsky]]

This is about to get backed out from cedar, because it's failing tests.  In particular, I believe it's failing these tests in content/media/test/test_can_play_type_wave.html (the actual test calls are in content/media/test/can_play_type_wave.js):

15   check("audio/wave; codecs=", "probably");
16   check("audio/wave; codecs=\"\"", "probably");

Comment 17

[Boris Zbarsky [:bzbarsky]]

Backed out from cedar.

Chris, could you take a look at this patch and the media test and see whether the problem is in the test or in the patch or in the media code?

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 524066 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

>diff --git a/netwerk/mime/nsMIMEHeaderParamImpl.cpp b/netwerk/mime/nsMIMEHeaderParamImpl.cpp
>--- a/netwerk/mime/nsMIMEHeaderParamImpl.cpp
>+++ b/netwerk/mime/nsMIMEHeaderParamImpl.cpp
>@@ -246,18 +268,24 @@ nsMIMEHeaderParamImpl::GetParameterInter
>     // If so, copy it and return.
>     if (tokenEnd - tokenStart == paramLen &&
>         !nsCRT::strncasecmp(tokenStart, aParamName, paramLen))
>     {
>       // if the parameter spans across multiple lines we have to strip out the
>       //     line continuation -- jht 4/29/98 
>       nsCAutoString tempStr(valueStart, valueEnd - valueStart);
>       tempStr.StripChars("\r\n");
>-      *aResult = ToNewCString(tempStr);
>-      NS_ENSURE_TRUE(*aResult, NS_ERROR_OUT_OF_MEMORY);

aResult is a char**

>+      char *res = ToNewCString(tempStr);

res is a char*

>+      NS_ENSURE_TRUE(*res, NS_ERROR_OUT_OF_MEMORY);

so this should be 

>+      NS_ENSURE_TRUE(res, NS_ERROR_OUT_OF_MEMORY);

Because if the string is "", as in the media test, *res==0, and so NS_ENSURE_TRUE returns NS_ERROR_OUT_OF_MEMORY in that case.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Ah, good catch!  Sounds like we should add that to the necko tests here too.

Comment 20

[Julian Reschke]

Will add more tests and fix.

Comment 21

[Julian Reschke]

Attachment: do backslash unescaping when the parameter uses quoted-string syntax

fixed the broken null check, added two related test cases

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 524851 [details] [diff] [review]
do backslash unescaping when the parameter uses quoted-string syntax

r+ for the NS_ENSURE_TRUE fix, that makes the test pass for me locally.

Comment 23

[Daniel Holbert [:dholbert]]

Landed:  http://hg.mozilla.org/mozilla-central/rev/5375d6280c92

I replaced "c += 1" with "++c" (per comment 4) in this line...
> for (char *c = src; *c; c += 1)
...before landing (with bz's approval over IRC)

Comment 24

[Eric Shepherd [:sheppy]]

Given that this header isn't documented anywhere yet to speak of, I've added a note about this change to Firefox 6 for developers:

https://developer.mozilla.org/en/Firefox_6_for_developers#HTTP

Comment 25

[Julian Reschke]

I just noticed that this caused an unplanned change in behavior for certain malformed headers, such as when there are multiple "filename" parameters.

See

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

(FF4 picks the first one, FF5aurora picks the second one)

I do not believe this should cause problems in practice, as this is a test case for an edge case, not a header seen in practice. My suggestion is to leave the fix in, but to be prepared to tune it (should this be raised as separate issue?).

Comment 26

[Julian Reschke]

(In reply to comment #25)
> I just noticed that this caused an unplanned change in behavior for certain
> malformed headers, such as when there are multiple "filename" parameters.
> ...

Wrong bug; see https://bugzilla.mozilla.org/show_bug.cgi?id=588781#c27 instead.