Closed Bug 1231309 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1327949, CID 1327948, CID 1327947)

Attachments

(1 file, 4 obsolete files)

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.
Attached patch Bug 1231309.diff (obsolete) — Splinter Review
Hello Bobby,

Can you please take a look other this patch?

THX
Attachment #8696938 - Flags: review?(bobbyholley)
Whiteboard: CID 1327949 → CID 1327949, CID 1327948, CID 1327947
Attached patch Bug 1231309 - CID 1327948.diff (obsolete) — Splinter Review
Same issue occurs in function XPCThrower::ThrowBadResult for sz
Attachment #8696940 - Flags: review?(bobbyholley)
Attached patch Bug 1231309 - CID 1327947.diff (obsolete) — Splinter Review
Also same issue in XPCThrower::Throw
Attachment #8696943 - Flags: review?(bobbyholley)
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 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.
Attachment #8696938 - Flags: review?(bobbyholley) → review-
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.
Attachment #8696940 - Flags: review?(bobbyholley)
Attachment #8696943 - Flags: review?(bobbyholley)
Attached patch Bug 1231309.diff (obsolete) — Splinter Review
Did you this had in mind?
Attachment #8696938 - Attachment is obsolete: true
Attachment #8696940 - Attachment is obsolete: true
Attachment #8696943 - Attachment is obsolete: true
Attachment #8697226 - Flags: review?(bobbyholley)
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.
Attachment #8697226 - Flags: review?(bobbyholley) → review+
Attached patch Bug 1231309.diffSplinter Review
Attachment #8697226 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8dfb9aeac3af
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.