Closed
Bug 1283064
Opened 8 years ago
Closed 8 years ago
[Static Analysis][Failure to restore non-local value] In function PrintCallIndirect
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox50 | --- | affected |
People
(Reporter: andi, Assigned: andi)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, Whiteboard: CID 1362640)
Attachments
(1 file)
The Static Analysis tool Coverity detected that failure to restore non-local value on variable c.currentPrecedence in the following context:
>> PrintOperatorPrecedence lastPrecedence = c.currentPrecedence;
>> c.currentPrecedence = ExpressionPrecedence;
>>
>> if (!PrintExpr(c, *call.index()))
>> return false;
>>
>> c.currentPrecedence = lastPrecedence;
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/61240/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/61240/
Attachment #8766264 -
Flags: review?(jorendorff)
Updated•8 years ago
|
Attachment #8766264 -
Flags: review?(jorendorff) → review-
Comment 2•8 years ago
|
||
Comment on attachment 8766264 [details] Bug 1283064 - restore correct value to |c.currentPrecedence| if PrintExpr fails. https://reviewboard.mozilla.org/r/61240/#review58154 I can see what Coverity is trying to do here, but on error, the WasmPrintContext is discarded. No code that uses a WasmPrintContext ever catches an error and uses the WasmPrintContext again. Furthermore the same idiom is used elsewhere in the file, so "fixing" it in just one place would be weird.
Comment 3•8 years ago
|
||
A general fix that we might take is some kind of temporarily-assign-a-value-but-save-the-old-value utility template: { ... SetTemporaryValue<PrintOperatorPrecedence> stv(c.currentPrecedence, ExpressionPrecedence); ... code that can fail ... } // `stv` has a destructor that puts the old value back, success or failure
Assignee | ||
Comment 4•8 years ago
|
||
yes in fact something similar is also present here:
>> MBasicBlock* loopBody = curBlock_;
>> curBlock_ = nullptr;
>>
>> // TODO (bug 1253544): blocks branching to the top join to a single
>> // backedge block. Could they directly be set as backedges of the loop
>> // instead?
>> if (!bindBranches(headerLabel))
>> return false;
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
Assignee | ||
Comment 5•8 years ago
|
||
This has already been pushed to m-c
You need to log in
before you can comment on or make changes to this bug.
Description
•