Closed Bug 1237272 Opened 4 years ago Closed 4 years ago

[Static Analysis][Dereference after null check] In function CheckModuleArguments(ModuleValidator& m, ParseNode* fn) from AsmJSValidate.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 1345987 )

Attachments

(1 file, 1 obsolete file)

The Static Analysis tool Coverity added that though arg1 is null check, it is dereferenced on the following lines without taking into consideration the null check:

>>    ParseNode* arg1 = FunctionArgsList(fn, &numFormals);
>>    ParseNode* arg2 = arg1 ? NextNode(arg1) : nullptr;
>>    ParseNode* arg3 = arg2 ? NextNode(arg2) : nullptr;
>>
>>    if (numFormals > 3)
>>        return m.fail(fn, "asm.js modules takes at most 3 argument");
>>
>>    PropertyName* arg1Name = nullptr;
>>    if (numFormals >= 1 && !CheckModuleArgument(m, arg1, &arg1Name))

In this case this easily can be considered a false-positive since the numFormulas is checked, but for precaution i would suggest adding an assert in the functional that ultimately null derefenced it - CheckArgument(ModuleValidator& m, ParseNode* arg, PropertyName** name): 

>>    if (!IsDefinition(arg))
>>        return m.fail(arg, "duplicate argument name not allowed");
>>
>>    if (arg->isKind(PNK_ASSIGN))
>>        return m.fail(arg, "default arguments not allowed");
Attached patch Bug 1237272.diff (obsolete) — Splinter Review
Attachment #8704633 - Flags: review?(jorendorff)
I'm not sure the MOZ_ASSERT helps very much since if this bug is made, it will immediately crash (in debug/release).  Is the static analysis happier if the "if (numFormals >= 1 &&" is changed to "if (arg1 &&"?
Attached patch Bug 1237272.diffSplinter Review
yes this is a better way of calming Coverity.
Attachment #8704633 - Attachment is obsolete: true
Attachment #8704633 - Flags: review?(jorendorff)
Attachment #8705030 - Flags: review?(luke)
Attachment #8705030 - Flags: review?(luke) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b71ca98d12c
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.