NS_NewURI damange non ASCII data

VERIFIED FIXED in M15

Status

()

Core
Networking
P3
blocker
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Frank Tang, Assigned: Warren Harris)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

18 years ago
Currently, NS_NewURI have the nsString favor of interface but it damage the 
non-ASCII data when it call ToCString.


 53             inline nsresult
 54             NS_NewURI(nsIURI* *result, const nsString& spec, nsIURI* baseURI 
= nsnull)
 55             {
 56                 // XXX if the string is unicode, GetBuffer() returns null. 
 57                 // XXX we need a strategy to deal w/ unicode specs (if there 
should
 58                 // XXX even be such a thing)
 59                 char* specStr = spec.ToNewCString(); // this forces a single 
byte char*
 60                 if (specStr == nsnull)
 61 warren  1.1         return NS_ERROR_OUT_OF_MEMORY;
 62                 nsresult rv = NS_NewURI(result, specStr, baseURI);
 63                 nsAllocator::Free(specStr);
 64                 return rv;
 65             }

Suggestion- Either remove the nsString favor to push the responsability out from 
Necko or call ToNewUTF8String to reserved Unicode data and let the protcol 
handler deal with it in individual protocol.

I don't think this is the *final* soultion for bug 14155 and 28171. But current 
call to ToCString is definitely wrong. Change it to call ToNewUTF8String may not 
solve the whole issue, but it have no possiblility to make it worst.
(Reporter)

Updated

18 years ago
Blocks: 14155
(Reporter)

Updated

18 years ago
No longer blocks: 14155

Comment 1

18 years ago
I vote for fixing NS_NewURI.

The call to ToNewCstring corrupts any non-ASCII data.
Changing the call ToNewUTF8String will not change data handling for the ASCII
case and will preserve the non-ASCII data.

Frank, Isn't this just a 1-line change?
(Assignee)

Updated

18 years ago
Target Milestone: M15
(Assignee)

Comment 2

18 years ago
There are 2 evil places in the nsNetUtil.h header that do that.
(Reporter)

Comment 3

18 years ago
This is a one liner and it is blocking QA finding more i18n problem. 
Keywords: beta1
(Reporter)

Comment 4

18 years ago
change to ToNewUTF8String and check in
Status: NEW → ASSIGNED

Comment 5

18 years ago
Created attachment 5440 [details] [diff] [review]
changes to protect against string corruption.

Comment 6

18 years ago
Can I get a review on this please?

Comment 7

18 years ago
whoops. we crossed paths. I'm marking this fixed. I also pulled the dead comment
out above one of the ToNewCString() calls.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 8

18 years ago
verified:  NT 2000042009
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.