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

RESOLVED WONTFIX

Status

()

Core
JavaScript Engine
RESOLVED WONTFIX
2 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
coverity
Points:
---

Firefox Tracking Flags

(firefox50 affected)

Details

(Whiteboard: CID 1362640)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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

2 years ago
Created attachment 8766264 [details]
Bug 1283064 - restore correct value to |c.currentPrecedence| if PrintExpr fails.

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)
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
(Assignee)

Comment 4

2 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

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 5

2 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.