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.

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

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

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?

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. :)