351418 - crash when |data| is null [@ strlen] [@ nsStringInputStream::SetData]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andreas Wuest]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.6) Gecko/20060728 Firefox/1.5.0.6

If we pass a null pointer to nsStringInputStream::SetData and dataLen is -1, we crash in strlen().

As for instance in:

(gdb) bt
#0  0x900030e8 in strlen ()
#1  0x1b508330 in nsStringInputStream::SetData (this=0x204465e0, data=0x0,
dataLen=-1) at
/Users/awuest-devel/Documents/devel/src/mozilla/trees/MOZILLA_1_8_0_BRANCH/mozilla/xpcom/io/nsStringStream.cpp:144

I wasn't sure if I should file it as a problem of strlen() (i.e., should strlen check for null pointers), or if the caller should make sure that strlen is never called with null (i.e. SetData would need a check).

Reproducible: Always

Steps to Reproduce:
1. call nsStringInputStream::SetData(null, -1)
2. crash

Actual Results:  
Crash in strlen().

Expected Results:  
nsStringInputStream::SetData should perhaps return with NS_ERROR_NULL_POINTER.

Comment 1

[Andreas Wuest]

BTW, I filed this as per https://bugzilla.mozilla.org/show_bug.cgi?id=283487#c18.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

null is not a valid input for strlen, it should crash. (what would you expect as the return value anyway?)

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

not a problem on trunk:
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsStringStream.cpp#175

Comment 4

[Andreas Wuest]

(In reply to comment #2)
> null is not a valid input for strlen, it should crash. (what would you expect
> as the return value anyway?)

Well, I don't know, and that's what I said. So therefore, SetData() has to do the checking.

(In reply to comment #3)
> not a problem on trunk:

But not on 1.8.1. Would it be desireable to also add the

NS_ENSURE_ARG_POINTER(data);

in http://lxr.mozilla.org/mozilla1.8/source/xpcom/io/nsStringStream.cpp#141 then (I could create a patch)?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, I think that would be good for 1_8 and 1_8_0. the other functions do check for null.

Comment 6

[Andreas Wuest]

Attachment: Patch against 1.8.0 and 1.8

Adds NS_ENSURE_ARG_POINTER(data); to nsStringInputStream::SetData(const char *data, PRInt32 dataLen). Based on 1.8.0, but looking at current 1.8, it should also easily apply there.

Comment 7

[Darin Fisher]

Comment on attachment 236815 [details] [diff] [review]
Patch against 1.8.0 and 1.8

a=darin on behalf of drivers for MOZILLA_1_8_BRANCH.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I got a=mconnor to land this now over IRC.
mozilla/xpcom/io/nsStringStream.cpp 	1.19.4.2

Comment 9

[Mike Schroepfer]

From bonsai this appears to have been fixed1.8.1 on 9-14 - marking as such

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 236815 [details] [diff] [review]
Patch against 1.8.0 and 1.8

approved for 1.8.0 branch, a=dveditz for drivers

Comment 11

[:Gavin Sharp [email: [e-mail]]]

mozilla/xpcom/io/nsStringStream.cpp 	1.19.12.1