Closed Bug 1307142 Opened 3 years ago Closed 3 years ago

[Static Analysis][Dereference after null check] In Init function generated from Codegen.py

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1373427, CID 1373584, CID 1373726)

Attachments

(1 file)

The Static Analysis tool Coverity detected that |cx| can be null deference in two situations:

1.

 >>  { // scope for isConvertible
 >>    bool isConvertible;
 >>    if (!IsConvertibleToDictionary(cx, val, &isConvertible)) {
 >>      return false;
 >>    }

2. 

 >>   if (!isConvertible) {
 >>      return ThrowErrorMessage(cx, MSG_NOT_DICTIONARY, sourceDescription);
 >>    }
 >>  }

Now in first case i think we can correlate the logic from IsConvertibleToDictionary with the:

 >> MOZ_ASSERT_IF(!cx, val.isNull());

What i propose is changing the assert to correlate with the code from IsConvertibleToDictionary:

>>  if (val.isNullOrUndefined()) {
>>    *convertible = true;
>>    return true;
>>  }

like:

 >> MOZ_ASSERT_IF(!cx, val.isNullOrUndefined());
Is the problem that it can't figure out the implication v.isNull() ==> v.isNullOrUndefined()? In any case, you should probably update the comment.
yes indeed it can't figure out it cannot figure out the implication of v.isNull() ==> v.isNullOrUndefined() and that's why i propose changing the assert to:
 
>> MOZ_ASSERT_IF(!cx, val.isNullOrUndefined());
Whiteboard: CID 1373427 → CID 1373427, CID 1373584
Whiteboard: CID 1373427, CID 1373584 → CID 1373427, CID 1373584, CID 1373726
Comment on attachment 8797183 [details]
Bug 1307142 - correlate MOZ_ASSERT_IF with logic from IsConvertibleToDictionary.

https://reviewboard.mozilla.org/r/82784/#review90392

::: dom/bindings/Codegen.py:12334
(Diff revision 1)
>  
>          """
>          body = dedent("""
>              // Passing a null JSContext is OK only if we're initing from null,
>              // Since in that case we will not have to do any property gets
> -            MOZ_ASSERT_IF(!cx, val.isNull());
> +            MOZ_ASSERT_IF(!cx, val.isNullOrUndefined());

Would it work to assert |val.isNull() && val.isNullOrUndefined()|? If so we'd prefer that instead.
Comment on attachment 8797183 [details]
Bug 1307142 - correlate MOZ_ASSERT_IF with logic from IsConvertibleToDictionary.

https://reviewboard.mozilla.org/r/82784/#review98016

::: dom/bindings/Codegen.py:12551
(Diff revision 2)
>  
>          """
>          body = dedent("""
>              // Passing a null JSContext is OK only if we're initing from null,
>              // Since in that case we will not have to do any property gets
> -            MOZ_ASSERT_IF(!cx, val.isNull());
> +            MOZ_ASSERT_IF(!cx, val.isNull() && val.isNullOrUndefined());

You should probably comment that we need both conditions to make coverity happy.
Attachment #8797183 - Flags: review?(peterv) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/534bd8b7b6d6
correlate MOZ_ASSERT_IF with logic from IsConvertibleToDictionary. r=peterv
https://hg.mozilla.org/mozilla-central/rev/534bd8b7b6d6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.