1231309 - [Static Analysis][Dereference before null check] In function XPCThrower::ThrowBadParam from XPCThrower.cpp

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that sz can be null since when it is dereferenced a null pointere dereference it will happen.
It is allocated bellow:

>>    sz = JS_smprintf("%s arg %d", format, paramNum);

>>    if (sz && sVerbose)
>>        Verbosify(ccx, &sz, true);

And dereferenced:

>>   dom::Throw(ccx, rv, nsDependentCString(sz));

nsDependentCString translates to:

>>explicit
>>nsTDependentString_CharT(const char_type* aData)
>>    : string_type(const_cast<char_type*>(aData),
>>                  uint32_t(char_traits::length(aData)), F_TERMINATED)
>>  {
>>    AssertValidDependentString();
>>  }

We should add an assert between allocation and dereference, on debug we can catch when nullptr is passed and maybe fix the effective problem, if there is any.

Comment 1

[Andi [:andi]]

Attachment: Bug 1231309.diff

Hello Bobby,

Can you please take a look other this patch?

THX

Comment 2

[Andi [:andi]]

Attachment: Bug 1231309 - CID 1327948.diff

Same issue occurs in function XPCThrower::ThrowBadResult for sz

Comment 3

[Andi [:andi]]

Attachment: Bug 1231309 - CID 1327947.diff

Also same issue in XPCThrower::Throw

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, good catch.  This is a regression from bug 1213289.

sz will only be null on OOM here, of course.  But we should still handle it.

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 8696938 [details] [diff] [review]
Bug 1231309.diff

Review of attachment 8696938 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCThrower.cpp
@@ +144,5 @@
>      if (sz && sVerbose)
>          Verbosify(ccx, &sz, true);
>  
> +    MOZ_ASSERT(sz, "sz should be a valid pointer!");
> +

This isn't right. If sz is null, we need to handle it immediately after it was assigned - probably by doing NS_ENSURE_TRUE(sz) after the JS_smprintf call.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8696940 [details] [diff] [review]
Bug 1231309 - CID 1327948.diff

Review of attachment 8696940 [details] [diff] [review]:
-----------------------------------------------------------------

Nit - please fold all of these into the same patch.

Comment 7

[Andi [:andi]]

Attachment: Bug 1231309.diff

Did you this had in mind?

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8697226 [details] [diff] [review]
Bug 1231309.diff

Review of attachment 8697226 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that.

::: js/xpconnect/src/XPCThrower.cpp
@@ +78,5 @@
>          format = "";
>  
>      sz = (char*) format;
>  
> +    NS_ENSURE_TRUE_VOID(sz);

Please remove the empty line between |sz = ...| and |NS_ENSURE_TRUE_VOID(sz);|.

@@ +121,5 @@
>          sz = JS_smprintf("%s 0x%x (%s)", format, result, name);
>      else
>          sz = JS_smprintf("%s 0x%x", format, result);
>  
> +    NS_ENSURE_TRUE_VOID(sz);

Same here.

@@ +144,5 @@
>          format = "";
>  
>      sz = JS_smprintf("%s arg %d", format, paramNum);
>  
> +    NS_ENSURE_TRUE_VOID(sz);

Same here.

Comment 9

[Andi [:andi]]

Attachment: Bug 1231309.diff

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dfb9aeac3af

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8dfb9aeac3af