Closed Bug 1248584 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1345979)

Attachments

(1 file)

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.
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?
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.
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 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/
Attachment #8719766 - Attachment description: MozReview Request: Bug 1248584 - if fallbackStub is not monitored exit from CloneOldBaselineStub with false. r?jandem → MozReview Request: Bug 1248584 - assert firstMonitorStub_ in ICMonitoredStub in order to silence Coverity. r?jandem
Attachment #8719766 - Flags: review?(jdemooij) → review+
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
https://hg.mozilla.org/mozilla-central/rev/1594c41619ff
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.