1168091 - Potential use of uninitialized class member js::frontend::FunctionBox::startLine in template <> bool Parser<SyntaxParseHandler>::finishFunctionDefinition(Node, FunctionBox* ,Node, Node)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bhargava Shastry]

Attachment: report-b541c0.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.65 Safari/537.36

Steps to reproduce:

Clang SA bug report attached.
If tokenStream.getToken(&tt) returns false, class member startLine will not be initialized.


Actual results:

Buggy line: LazyScript::CreateRaw(context, fun, numFreeVariables, numInnerFunctions, versionNumber(), funbox->bufStart, funbox->bufEnd, funbox->startLine, funbox->startColumn)

in Function:
Parser<SyntaxParseHandler>::finishFunctionDefinition(Node, FunctionBox* , Node , Node)


Potential call stack [Innermost call first]:

js::frontend::Parser<js::frontend::SyntaxParseHandler>::finishFunctionDefinition(js::frontend::SyntaxParseHandler::Node, js::frontend::FunctionBox*, js::frontend::SyntaxParseHandler::Node, js::frontend::SyntaxParseHandler::Node)

js::frontend::Parser<js::frontend::SyntaxParseHandler>::functionArgsAndBodyGeneric(js::frontend::InHandling, js::frontend::YieldHandling, js::frontend::SyntaxParseHandler::Node, JS::Handle<JSFunction*>, js::frontend::FunctionSyntaxKind)

js::frontend::Parser<js::frontend::FullParseHandler>::functionArgsAndBody(js::frontend::InHandling, js::frontend::ParseNode*, JS::Handle<JSFunction*>, js::frontend::FunctionSyntaxKind, js::GeneratorKind, js::frontend::Directives, js::frontend::Directives*)

js::frontend::Parser<js::frontend::FullParseHandler>::functionDef(js::frontend::InHandling, js::frontend::YieldHandling, JS::Handle<js::PropertyName*>, js::frontend::FunctionSyntaxKind, js::GeneratorKind, js::frontend::Parser<js::frontend::FullParseHandler>::InvokedPrediction)

js::frontend::Parser<js::frontend::FullParseHandler>::functionStmt(js::frontend::YieldHandling)

js::frontend::Parser<js::frontend::FullParseHandler>::statement(js::frontend::YieldHandling, bool)

js::frontend::Parser<js::frontend::FullParseHandler>::statements(js::frontend::YieldHandling)

js::frontend::Parser<js::frontend::FullParseHandler>::parse(JSObject*)


Expected results:

Initialize startLine earlier, possibly before call to getToken.

Comment 1

[Andrew McCreight [:mccr8]]

Jeff, can you look into this? Thanks.

Comment 2

[Andrew McCreight [:mccr8]]

Brian, could you look at this? It looks related to lazy parsing. Off-hand I would guess that the line number of a script being random junk wouldn't cause security issues, but I can't tell for sure. Thanks.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: initialize members

This isn't related to lazy parsing.  The FunctionBox constructor initializes all members except startLine and startColumn, and expects setStart to be called on it at some point.  I don't know what getToken() call the static analysis is referring to, but if that function returns false then we ran out of memory or hit an illegal token and will be unwinding the stack without trying to do any more work.  So I think this is a false positive and even if it were a true positive it should be benign --- the line number and column are I think only used for error reporting.  But all in all it's best to just fix the sloppiness in FunctionBox's constructor, as this patch does.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8663321 [details] [diff] [review]
initialize members

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

Yep, agreed all the way through. Default startLine to 1, though.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9350460480

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ae9350460480

Comment 7

[Bhargava Shastry]

Just curious: Does this bug report get a security rating because of the garbage read?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Bhargava Shastry from comment #7)
> Just curious: Does this bug report get a security rating because of the
> garbage read?

From comment 3, it sounds like that this uninitialized read would only result in a bogus line or column number being reported for some JS code, so it is not a security issue.