1248584 - [Static Analysis][Explicit null dereferenced] In function CloneOldBaselineStub

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andi-Bogdan Postelnicu [:andi]]

The Static Analysis tool Coverity added that firstMonitorStub is null pointer dereference when trying to clone in swtich code block.  This can happen if the following if is false:

>>    ICStub* firstMonitorStub;
>>    if (fallbackStub->isMonitoredFallback()) {
>>        ICMonitoredFallbackStub* monitored = fallbackStub->toMonitoredFallbackStub();
>>        firstMonitorStub = monitored->fallbackMonitorStub()->firstMonitorStub();
>>    } else {
>>        firstMonitorStub = nullptr;
>>    }

The place where it actually gets dereferenced is:

>>    switch (oldStub->kind()) {
>>#define CASE_KIND(kindName)                                                  \
>>      case ICStub::kindName:                                                 \
>>        entry.newStub = IC##kindName::Clone(cx, stubSpace, firstMonitorStub, \
>>                                            *oldStub->to##kindName());       \
>>        break;
>>        PATCHABLE_ICSTUB_KIND_LIST(CASE_KIND)
>>#undef CASE_KIND
>>
>>      default:
>>        MOZ_CRASH("Bad stub kind");
>>    }

Looking through the ICs that are specified here and call clone all of the call func tion New<T>

>>    template <typename T, typename... Args>
>>    static T* New(JSContext* cx, ICStubSpace* space, JitCode* code, Args&&... args) {
>>        if (!code)
>>            return nullptr;
>>        T* result = space->allocate<T>(code, mozilla::Forward<Args>(args)...);
>>        if (!result)
>>            ReportOutOfMemory(cx);
>>        return result;
>>    }

and as it can be seen space that is taken the value from firstMonitorStub  is dereferenced this in case firstMonitorStub  is nullptr leading to a nullptr dereference.

Comment 1

[Andi-Bogdan Postelnicu [:andi]]

Attachment: MozReview Request: Bug 1248584 - assert firstMonitorStub_ in ICMonitoredStub in order to silence Coverity. r?jandem

Review commit: https://reviewboard.mozilla.org/r/35099/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35099/

Comment 2

[Jan de Mooij [:jandem]]

I don't understand the problem. Do you have an error message or stack from Coverity?

(In reply to Andi-Bogdan Postelnicu from comment #0)
> and as it can be seen space that is taken the value from firstMonitorStub 
> is dereferenced this in case firstMonitorStub  is nullptr leading to a
> nullptr dereference.

firstMonitorStub is not passed as |space|, but as ICStub* argument right?

Comment 3

[Andi-Bogdan Postelnicu [:andi]]

Coverity is just a static analysis tools so there is no stack trace. What coveirty warns is that if in case firstMonitorStub  is null, a null pointer dereference will occur  in function New that is called by IC##kindName::Clone :

>>    template <typename T, typename... Args>
>>    static T* New(JSContext* cx, ICStubSpace* space, JitCode* code, Args&&... args) {
>>        if (!code)
>>            return nullptr;
>>        T* result = space->allocate<T>(code, mozilla::Forward<Args>(args)...);
>>        if (!result)
>>            ReportOutOfMemory(cx);
>>        return result;
>>    }

In the context of New function variable |space| is actually variable |firstMonitorStub| from CloneOldBaselineStub.

Comment 4

[Andi-Bogdan Postelnicu [:andi]]

It seems i wasn't that exact in my last comments, you can see that on below lines the null pointer dereference takes place:

>>ICMonitoredStub::ICMonitoredStub(Kind kind, JitCode* stubCode, ICStub* firstMonitorStub)
>>  : ICStub(kind, ICStub::Monitored, stubCode),
>>    firstMonitorStub_(firstMonitorStub)
>>{
>>    // If the first monitored stub is a ICTypeMonitor_Fallback stub, then
>>    // double check that _its_ firstMonitorStub is the same as this one.
>>    MOZ_ASSERT_IF(firstMonitorStub_->isTypeMonitor_Fallback(),
>>                  firstMonitorStub_->toTypeMonitor_Fallback()->firstMonitorStub() ==
>>                     firstMonitorStub_);
>>}

Comment 5

[Andi-Bogdan Postelnicu [:andi]]

Comment on attachment 8719766 [details]
MozReview Request: Bug 1248584 - assert firstMonitorStub_ in ICMonitoredStub in order to silence Coverity. r?jandem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35099/diff/1-2/

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8719766 [details]
MozReview Request: Bug 1248584 - assert firstMonitorStub_ in ICMonitoredStub in order to silence Coverity. r?jandem

https://reviewboard.mozilla.org/r/35099/#review31925

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1594c41619ff

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1594c41619ff