518001 - Null deref in nsCharTraits::size

Status: RESOLVED INVALID

(Core :: XPCOM, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: tentative fix

Not sure whether dbaron or bsmedberg (or both) should review, please advise.

Comment 2

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

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?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 4

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

Why should we be checking for incorrect code at runtime every time we construct a string?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 6

[Benjamin Smedberg]

Comment on attachment 402002 [details] [diff] [review]
tentative fix

Then why not just do that?