Open Bug 1258379 Opened 4 years ago Updated 4 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox48 --- affected


(Reporter: andi, Assigned: jorendorff)


(Blocks 1 open bug)


(Keywords: coverity, Whiteboard: CID 1357072 )


(2 files)

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.
Duplicate of this bug: 1258348
Comment on attachment 8732864 [details]
MozReview Request: Bug 1258379 - prevent null pointer derefence on |last|. r?jorendorff

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)) {
Attachment #8732864 - Flags: review?(jorendorff)
Assignee: bogdan.postelnicu → 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 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?
Attachment #8732960 - Flags: review?(shu) → review+
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. :)
You need to log in before you can comment on or make changes to this bug.