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)
9 years ago
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 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.