141866 - NS_ConvertUTF8toUCS2(ptr, length) doesn't correctly handle given length

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Mike Pinkerton (not reading bugmail)]

in trying to use
      NS_ConvertUTF8toUCS2( const char* aCString, PRUint32 aLength )
        {
          Init( nsDependentCString( aCString, aLength ) );
        }

I discovered that even if aCString is null terminated, nsDependentCString will
assert in Rebind if the null comes after |aLength| bytes. 

this blocks work in 69566

Comment 1

[Mike Pinkerton (not reading bugmail)]

topembed because it blocks a topembed bug

Comment 2

[jag (Peter Annema)]

Attachment: Make NS_ConvertUTF8toUCS2(ptr, length) work with substrings

The problem isn't with nsDependentCString, that does exactly what it should.
The problem is that we didn't allow a substring of the |const char*| to be
passed in.

Comment 3

[Scott Collins]

Comment on attachment 82131 [details] [diff] [review]
Make NS_ConvertUTF8toUCS2(ptr, length) work with substrings

sr=scc, and thanks for the fix, jag

Comment 4

[jag (Peter Annema)]

Updating summary.

Comment 5

[Mike Pinkerton (not reading bugmail)]

this really does block 69566

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 82131 [details] [diff] [review]
Make NS_ConvertUTF8toUCS2(ptr, length) work with substrings

r=pink.

i'm going to land this.

Comment 7

[Mike Pinkerton (not reading bugmail)]

landed on trunk. we need this on the branch for 69566

Comment 8

[Mike Pinkerton (not reading bugmail)]

actually, this patch doesn't work.

Comment 9

[Mike Pinkerton (not reading bugmail)]

removing adt keyword. it appears we were premature. the patch doesn't work.
also, don't a whole bunch of other things that use nsDependent*String(foo, len)
have to be changed as well?

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Other things that use nsDependent[C]String(str,len) should already have been
fixed.  The patch certainly should fix the assertion, and seems like the right
thing to do.  What doesn't work about it?  (nsDependentCString is an
nsAFlatCString, which implies that the buffer is null-terminated at its length.
 |Substring| returns an object that is an nsASingleFragmentCString (one higher
in the hierarchy), which doesn't imply null-termination.)

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(That is, they should have been fixed if the |len| that they were providing was
there to indicate that a substring was desired rather than that the length of
the string was already known.)

Comment 12

[Mike Pinkerton (not reading bugmail)]

i'm still getting the assert about it having to be null terminated, even with
the change, but then again the dependency checking in the win32 gmake build is
so bad, it might be a phantom. i'm doing a full rebuild now just to be certain.

Comment 13

[Mike Pinkerton (not reading bugmail)]

did a full rebuild, it still asserts, but now with a different assert.

if i don't give it a length, it's ok. if i do give it a length, it asserts now
that this can only be used with a known UTF8 string (which it is). What's the
difference?

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What's the stack for the new assertion?  And what's the input string?

Comment 15

[jag (Peter Annema)]

Hmmm... That sounds like your buffer contains a character it can't decode.

Comment 16

[jag (Peter Annema)]

If you don't give NS_ConvertUTF8toUCS2 a length, it'll go find the zero
terminator and determine the length of the buffer that way. It's the equivalent
of doing |NS_ConvertUTF8toUCS2(str, strlen(str))|.

So, since it can decode the utf8 buffer just fine if you don't give it a length,
it sounds like you're cutting some utf8 char in half (or whatever) with the
length you've given.

Comment 17

[Mike Pinkerton (not reading bugmail)]

hrm, this is just a simple ascii string, all characters are in the range 00 ->
7F. i dont see any multi-byte characters anywhere in my string.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So what's the stack for the new assertion?  We're only guessing at which
assertion you might have hit.

Comment 19

[Mike Pinkerton (not reading bugmail)]

landed. we need this on the branch for embedding (for 69566)

Comment 20

[scottputterman]

adding adt1.0.0+.  Please get drivers approval before checking into the 1.0 branch.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 82131 [details] [diff] [review]
Make NS_ConvertUTF8toUCS2(ptr, length) work with substrings

a=[e-mail] (make sure it's in trunk too)

Comment 22

[Mike Pinkerton (not reading bugmail)]

landed on 1.0.0 branch on 5/16/02