Closed Bug 1283064 Opened 4 years ago Closed 4 years ago

[Static Analysis][Failure to restore non-local value] In function PrintCallIndirect

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

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;
Attachment #8766264 - Flags: review?(jorendorff) → review-
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.
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
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;
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
This has already been pushed to m-c
You need to log in before you can comment on or make changes to this bug.