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

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Andi [:andi]]

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

Comment 1

[Andi [:andi]]

Attachment: Bug 1237291.diff

Comment 2

[Jason Orendorff [:jorendorff]]

This is definitely some squirrely code. Trying to figure out how best to fix it...

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Silence Coverity warning about missing null check

Comment 4

[Jason Orendorff [:jorendorff]]

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.

Comment 5

[Shu-yu Guo [:shu]]

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.

Comment 6

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd803e8713ea

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.