Closed Bug 1235032 Opened 4 years ago Closed 4 years ago

[Static Analysis][Dereference before null check] In function GetPropertyIC::canAttachDenseElementHole from IonCaches.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1345752 )

Attachments

(1 file, 1 obsolete file)

The Static Analysis tool Coverity added that a dereference before null check is present on variable obj. It is dereferenced here:

>>    if (!obj->isNative())
>>        return false;
>>
>>    if (obj->as<NativeObject>().getDenseInitializedLength() == 0)
>>        return false; 

the check if here:

>>    while (obj) {

Sure the check is for looping purpose and obj value is retrieved via the operator JSObject* from Handle<JSObject*> when it's pushed on the stack for canAttachDenseElementHole. As the constructor of handle<JSObject*> accepts null pointer as argument, i would suggest adding an assert for obj variable, in this way we can silence Coverity and also during automation tests it will trigger if null is passed.
If we don't want to have an assert and we are sure of the fact that we will not have null for obj we can change the loop from while(obj) to do {} while(obj);
Attached patch Bug 1235032.diff (obsolete) — Splinter Review
Attachment #8701798 - Flags: review?(jorendorff)
Comment on attachment 8701798 [details] [diff] [review]
Bug 1235032.diff

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

Stealing; Jason is on PTO until January.

I expect decent compilers to turn this into a do-while anyway, but if this shuts up Coverity, sure.

::: js/src/jit/IonCaches.cpp
@@ +3812,5 @@
>  /* static */ bool
>  GetPropertyIC::canAttachDenseElementHole(JSObject* obj, HandleValue idval, TypedOrValueRegister output)
>  {
> +    MOZ_ASSERT(obj, "obj must be valid pointer");
> +

Please remove these two lines; the calls below (like obj->isNative()) will crash when obj is nullptr, so in these cases we usually don't add an assert.
Attachment #8701798 - Flags: review?(jorendorff) → review+
Attached patch Bug 1235032.diffSplinter Review
Assert removed
Attachment #8701798 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/38cae0ef3a70
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.