Null deref in nsCharTraits::size

RESOLVED INVALID

Status

()

Core
String
RESOLVED INVALID
9 years ago
9 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Trunk
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

If one tries calls |nsDependentCString(NULL)|, Gecko crashes from a null deref in libc.  The relevant backtrace is

(gdb) bt
#0  0x00007f198bbafc60 in strlen () from /lib/libc.so.6
#1  0x00007f198d43195b in nsCharTraits<char>::length (s=0x0) at ../../../dist/include/nsCharTraits.h:629
#2  0x00007f198d4319a3 in nsDependentCString (this=0x7f1983efaeb0, data=0x0) at ../../../dist/include/nsTDependentString.h:89

My guess is that null |data| pointers were intended to have been caught by the |AssertValid()| check in the nsDependentCString ctor body.  So I will propose a fix to nsCharTraits<T>::length().

Please advise if another fix is preferred.
Created attachment 402002 [details] [diff] [review]
tentative fix

Not sure whether dbaron or bsmedberg (or both) should review, please advise.
Attachment #402002 - Flags: review?(dbaron)
Attachment #402002 - Flags: review?(benjamin)
The original answer (when this code was written) was that one wasn't supposed to call nsDependentCString(NULL).  Is there a reason we want to allow it?
No, it shouldn't be allowed.  The question is what kind of error it should trigger.

  nsDependentString(NULL, 0) ==> assertion failure
  nsDependentString(NULL)    ==> crash

This patch makes the second case fail an assertion, like the first.  I would be all for using NS_ABORT_IF_FALSE() or even NS_RUNTIMEABORT() for both, if that's the desired behavior.
Why should we be checking for incorrect code at runtime every time we construct a string?
We certainly don't have to.

I filed this bug assuming it's undesirable for ns*DependentString to fail in different ways when invariants don't hold, depending on the ctor (crash vs. assertion failure).  Apparently that's an invalid assumption.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → INVALID

Comment 6

9 years ago
Comment on attachment 402002 [details] [diff] [review]
tentative fix

Then why not just do that?
Attachment #402002 - Flags: review?(benjamin) → review-
Attachment #402002 - Flags: review?(dbaron) → review-
You need to log in before you can comment on or make changes to this bug.