Closed
Bug 588389
Opened 14 years ago
Closed 13 years ago
content-disposition: escaped characters in quoted-string give funny results
Categories
(Core Graveyard :: File Handling, defect)
Core Graveyard
File Handling
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla6
People
(Reporter: julian.reschke, Assigned: julian.reschke)
References
(Blocks 1 open bug, )
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 3 obsolete files)
4.36 KB,
patch
|
cpearce
:
feedback-
|
Details | Diff | Splinter Review |
4.65 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
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"
Updated•14 years ago
|
Component: General → File Handling
Product: Firefox → Core
QA Contact: general → file-handling
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #473519 -
Flags: review?(cbiesinger)
Updated•13 years ago
|
Attachment #473519 -
Flags: review?(cbiesinger) → review?(bzbarsky)
Comment 3•13 years ago
|
||
Not sure if this should be File Handling or Networking...
Assignee: nobody → julian.reschke
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
![]() |
||
Comment 4•13 years ago
|
||
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.
Attachment #473519 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #473023 -
Attachment is obsolete: true
Attachment #473519 -
Attachment is obsolete: true
Attachment #481228 -
Flags: review?(bzbarsky)
![]() |
||
Comment 6•13 years ago
|
||
Comment on attachment 481228 [details] [diff] [review] do backslash unescaping then the parameter uses quoted-string syntax r=me
Attachment #481228 -
Flags: review?(bzbarsky) → review+
Updated•13 years ago
|
Attachment #481228 -
Flags: approval2.0?
Comment 7•13 years ago
|
||
What's the risk and reward here? Offhand it doesn't look like this is worth taking now.
Comment 8•13 years ago
|
||
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.
Attachment #481228 -
Flags: approval2.0?
Assignee | ||
Comment 9•13 years ago
|
||
(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.
Assignee | ||
Comment 11•13 years ago
|
||
Probably as it is in conflict with the change for 588781; will check.
Assignee | ||
Comment 12•13 years ago
|
||
this includes a stand-alone JS test file; maybe it should be integrated into Jason's test_MIME_params.js?
Attachment #481228 -
Attachment is obsolete: true
Assignee | ||
Comment 13•13 years ago
|
||
(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 15•13 years ago
|
||
Ehsan says this has landed on cedar, on its way to mozilla-central.
![]() |
||
Comment 16•13 years ago
|
||
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•13 years ago
|
||
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?
![]() |
||
Updated•13 years ago
|
Attachment #524066 -
Flags: feedback?(chris)
![]() |
||
Updated•13 years ago
|
Keywords: checkin-needed
Comment 18•13 years ago
|
||
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.
Attachment #524066 -
Flags: feedback?(chris) → feedback-
![]() |
||
Comment 19•13 years ago
|
||
Ah, good catch! Sounds like we should add that to the necko tests here too.
Assignee | ||
Comment 20•13 years ago
|
||
Will add more tests and fix.
Assignee | ||
Comment 21•13 years ago
|
||
fixed the broken null check, added two related test cases
Updated•13 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Updated•13 years ago
|
Attachment #524851 -
Flags: review?(chris)
Comment 22•13 years ago
|
||
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.
Attachment #524851 -
Flags: review?(chris) → review+
Comment 23•13 years ago
|
||
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)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Version: unspecified → Trunk
Comment 24•13 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 25•13 years ago
|
||
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?).
Assignee | ||
Comment 26•13 years ago
|
||
(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.
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•