1258379 - [Static Analysis][Dereference before null check] In function Parser<ParseHandler>::bindVar

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that variable |last| is dereferenced before it is null checked leading to a possible null pointer derererence:

>>        DefinitionNode last = pc->decls().lookupLast(name);
>>        Definition::Kind lastKind = parser->handler.getDefinitionKind(last);
>>        if (last && lastKind != Definition::VAR && lastKind != Definition::ARG) {
>>            parser->handler.setFlag(parser->handler.getDefinitionNode(last), PND_CLOSED);
>>
>>            Node synthesizedVarName = parser->newName(name);
>>            if (!synthesizedVarName)
>>                return false;
>>            if (!pc->define(parser->tokenStream, name, synthesizedVarName, Definition::VAR,
>>                            /* declaringVarInCatchBody = */ true))
>>            {
>>                return false;
>>            }
>>        }

looking throug code i'm not convinced that |last| will always be a valid pointer, so i propose checked the validity of |last| before dereferencing it.

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1258379 - prevent null pointer derefence on |last|. r?jorendorff

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

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8732864 [details]
MozReview Request: Bug 1258379 - prevent null pointer derefence on |last|. r?jorendorff

https://reviewboard.mozilla.org/r/41411/#review37913

Declining review here - there's a better way. I'll whip up a patch and r?shu.

::: js/src/frontend/Parser.cpp:4100
(Diff revision 1)
>      DefinitionList::Range defs = pc->decls().lookupMulti(name);
>      MOZ_ASSERT_IF(stmt, !defs.empty());
>  
>      if (defs.empty())
>          return pc->define(parser->tokenStream, name, pn, Definition::VAR);
>  

This if-statement ensures that if we proceed, there is an entry in `pc->decls()` for `name` and therefore the lookup below can't miss. So I think the correct fix is to reuse `defs` rather than redoing the lookup.

::: js/src/frontend/Parser.cpp:4105
(Diff revision 1)
>  
>      // ES6 Annex B.3.5 allows for var declarations inside catch blocks with
>      // the same name as the catch parameter.
>      bool nameIsCatchParam = stmt && stmt->type == StmtType::CATCH;
>      bool declaredVarInCatchBody = false;
>      if (nameIsCatchParam && !data->isForOf && !HasOuterLexicalBinding(pc, stmt, name)) {

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Silence Coverity warning about a missing nullcheck in Parser<FPH>::bindVar()

Comment 5

[Jason Orendorff [:jorendorff]]

Reusing defs turns out to be a huge pain, so I changed it to assert instead of testing `last`. This should satisfy Coverity.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8732960 [details] [diff] [review]
Silence Coverity warning about a missing nullcheck in Parser<FPH>::bindVar()

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

::: js/src/frontend/ParseMaps.h
@@ +472,5 @@
>      /* Return the definition at the tail of the chain for |atom|. */
>      DefinitionNode lookupLast(JSAtom* atom) const {
>          MOZ_ASSERT(map);
>          DefinitionList::Range range = lookupMulti(atom);
> +        MOZ_ASSERT(!range.empty());

In the one use of this method in Parser.cpp, the range can never be empty. As a general purpose method though, it can very well be empty. Please move the assertion that dn is not null to Parser.cpp. Does that satisfy Coverity?

Comment 7

[Jason Orendorff [:jorendorff]]

There are three uses of this method in Parser.cpp, and in each case it's used only when a previous lookup (often lookupFirst) already found a binding, so the answer can't be null.

I'll leave the method as "general purpose", as requested... arguably, though, if there's ever a fourth call site for this method something has gone horribly wrong. :)

Comment 8

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

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