Last Comment Bug 588389 - content-disposition: escaped characters in quoted-string give funny results
: content-disposition: escaped characters in quoted-string give funny results
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core Graveyard
Classification: Graveyard
Component: File Handling (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Julian Reschke
:
:
Mentors:
http://greenbytes.de/tech/tc2231/#att...
Depends on:
Blocks: 609667
  Show dependency treegraph
 
Reported: 2010-08-18 06:12 PDT by Julian Reschke
Modified: 2016-06-22 12:16 PDT (History)
9 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
test case for unescaping quoted-string (958 bytes, text/javascript)
2010-09-08 05:43 PDT, Julian Reschke
no flags Details
do backslash unescaping then the parameter uses quoted-string syntax (4.32 KB, patch)
2010-09-09 06:52 PDT, Julian Reschke
bzbarsky: review+
Details | Diff | Splinter Review
do backslash unescaping then the parameter uses quoted-string syntax (4.40 KB, patch)
2010-10-06 08:30 PDT, Julian Reschke
bzbarsky: review+
Details | Diff | Splinter Review
do backslash unescaping then the parameter uses quoted-string syntax (4.36 KB, patch)
2011-04-05 09:36 PDT, Julian Reschke
cpearce: feedback-
Details | Diff | Splinter Review
do backslash unescaping when the parameter uses quoted-string syntax (4.65 KB, patch)
2011-04-09 08:23 PDT, Julian Reschke
cpearce: review+
Details | Diff | Splinter Review

Description Julian Reschke 2010-08-18 06:12:47 PDT
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 2010-09-08 05:43:59 PDT
Created attachment 473023 [details]
test case for unescaping quoted-string
Comment 2 Julian Reschke 2010-09-09 06:52:05 PDT
Created attachment 473519 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax
Comment 3 Reed Loden [:reed] (use needinfo?) 2010-10-05 00:02:55 PDT
Not sure if this should be File Handling or Networking...
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2010-10-05 21:45:37 PDT
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 2010-10-06 08:30:11 PDT
Created attachment 481228 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2010-10-06 08:35:03 PDT
Comment on attachment 481228 [details] [diff] [review]
do backslash unescaping then the parameter uses quoted-string syntax

r=me
Comment 7 Benjamin Smedberg [:bsmedberg] 2010-11-01 07:49:47 PDT
What's the risk and reward here? Offhand it doesn't look like this is worth taking now.
Comment 8 Benjamin Smedberg [:bsmedberg] 2010-11-10 13:05:31 PST
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 2010-11-10 23:22:22 PST
(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 :Ehsan Akhgari 2011-03-25 15:03:00 PDT
This patch doesn't apply cleanly on trunk any more.
Comment 11 Julian Reschke 2011-04-05 07:01:11 PDT
Probably as it is in conflict with the change for 588781; will check.
Comment 12 Julian Reschke 2011-04-05 09:36:01 PDT
Created attachment 524066 [details] [diff] [review]
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 2011-04-08 05:30:56 PDT
(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 [:bsmedberg] 2011-04-08 07:45:07 PDT
Yes, this can land.
Comment 15 Christopher Blizzard (:blizzard) 2011-04-08 13:45:39 PDT
Ehsan says this has landed on cedar, on its way to mozilla-central.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-04-08 13:53:57 PDT
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 [:bz] (still a bit busy) 2011-04-08 14:08:14 PDT
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) 2011-04-08 15:21:43 PDT
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 [:bz] (still a bit busy) 2011-04-08 15:56:19 PDT
Ah, good catch!  Sounds like we should add that to the necko tests here too.
Comment 20 Julian Reschke 2011-04-09 03:38:09 PDT
Will add more tests and fix.
Comment 21 Julian Reschke 2011-04-09 08:23:32 PDT
Created attachment 524851 [details] [diff] [review]
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) 2011-04-13 19:49:14 PDT
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] 2011-04-14 12:32:00 PDT
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] 2011-04-14 14:41:27 PDT
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 2011-04-16 06:21:25 PDT
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 2011-04-17 00:36:11 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.