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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andi [:andi]]

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);

Comment 1

[Andi [:andi]]

Attachment: Bug 1235032.diff

Comment 2

[Jan de Mooij [:jandem]]

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.

Comment 3

[Andi [:andi]]

Attachment: Bug 1235032.diff

Assert removed

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38cae0ef3a70

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38cae0ef3a70