267611 - get rid of malloc/copy/free sequence in nsStandardURL::Init

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Brian Ryner (not reading)]

Currently, if |charset| is "UTF-*", we hit a pretty bad case for string
operations in nsStandardURL::Init.  We'll malloc a heap buffer for
mOriginCharset, copy the given string into it, then free the buffer once we see
that it starts with "UTF".  Much better to figure that out _before_ messing with
mOriginCharset.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Comment 2

[Darin Fisher]

Comment on attachment 164519 [details] [diff] [review]
patch

>Index: nsStandardURL.cpp

>+// URI can't be encoded in UTF-16, UTF-16BE, UTF-16LE, UTF-32, UTF-32-LE,
>+// UTF-32LE, UTF-32BE (yet?). Truncate mOriginCharset if it starts with
>+// "utf" (since an empty mOriginCharset implies UTF-8, this is safe even if
>+// mOriginCharset is UTF-8).
>+
>+inline PRBool
>+IsUTFCharset(const char *aCharset)

moving the comment above this function doesn't quite make sense.  i think
the comment should stay down where we actually assign mOriginCharset.


>+            if (mOriginCharset.Length() >= 3 &&
>+                IsUTFCharset(mOriginCharset.get())) {
>+                mOriginCharset.Truncate();

I think you can safely check mOriginCharset.Length() > 3 since
"UTF" should be accompanied by a suffix.


>+    else {
>+        if (!IsUTFCharset(charset)) {
>+            mOriginCharset = charset;
>+        }
>+    }

nit: you could write:

      else if (!IsUTFCharset(charset))
	  mOriginCharset = charset;


r=darin with those nits fixed.

Comment 3

[Simon Montagu :smontagu]

I think this caused the crash in bug 271196.

Comment 4

[Florian Quèze [:florian]]

Shouldn't this be marked FIXED?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This got checked in a LONG time ago and the only regression it caused is also long fixed. Marking FIXED.