Open Bug 1237291 Opened 4 years ago Updated 4 years ago

[Static Analysis][Dereference after null check] In function Parser<FullParseHandler>::checkFunctionDefinition from Parser.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

ASSIGNED
Tracking Status
firefox46 --- affected

People

(Reporter: andi, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1345984)

Attachments

(2 files)

The Static Analysis tool Coverity added that stmt is dereferenced after null check:
>>                if (stmt && stmt->type != StmtType::BLOCK && stmt->type != StmtType::SWITCH) {
>>                    report(ParseError, false, null(), JSMSG_SLOPPY_FUNCTION_LABEL);
>>                    return false;
>>                }

dereferece:
>> bodyLevelFunction = pc->atBodyLevel(stmt);

and in function atBodyLevel:

>>        if (sc->staticScope()->is<StaticEvalObject>()) {
>>            bool bl = !stmt->enclosing;
>>            MOZ_ASSERT_IF(bl, stmt->type == StmtType::BLOCK);
>>            MOZ_ASSERT_IF(bl, stmt->staticScope

Looking at the code i think in case stmt is not a valid pointer we can have bodyLevelFunction = false. if this is not the case and my assumption is wrong but we know for sure that stmt is a valid pointer we should remove it from 
>>                if (stmt && stmt->type != StmtType::BLOCK && stmt->type != StmtType::SWITCH) {
Attached patch Bug 1237291.diffSplinter Review
Attachment #8704666 - Flags: review?(jorendorff)
This is definitely some squirrely code. Trying to figure out how best to fix it...
Assignee: bogdan.postelnicu → jorendorff
Status: NEW → ASSIGNED
Comment on attachment 8704666 [details] [diff] [review]
Bug 1237291.diff

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

Thanks for the patch - again I think we'll fix it in a slightly different way. Clearing the review flag.

::: js/src/frontend/Parser.cpp
@@ +2395,3 @@
>                  }
> +                else {
> +                    bodyLevelFunction = false;

I think false is the wrong answer here - if there's no enclosing statement, then the statement *is* at body level.
Attachment #8704666 - Flags: review?(jorendorff)
Comment on attachment 8732983 [details] [diff] [review]
Silence Coverity warning about missing null check

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

::: js/src/frontend/Parser.cpp
@@ +2426,5 @@
>                      report(ParseError, false, null(), JSMSG_SLOPPY_FUNCTION_LABEL);
>                      return false;
>                  }
>  
> +                bodyLevelFunction = !stmt || pc->atBodyLevel(stmt);

atBodyLevel basically *is* the check |!stmt| except for the weird eval special case, in which case we know stmt is non-null. If it silences Coverity there should be a |MOZ_ASSERT(stmt)| in the |if (sc->staticScope()->is<StaticEvalScope>())| case in atBodyLevel.
Attachment #8732983 - Flags: review?(shu)
You need to log in before you can comment on or make changes to this bug.