Terser: assignment in switch statement corrupts program

[aeschli]

Terser 4.2.1

• git clone https://github.com/aeschli/sample-terser-bug

• cd sample-terser-bug, npm i, npm test

• compiles a sample module

• created a minified variant (see https://github.com/aeschli/sample-terser-bug/blob/662548260936142a48e9c0d2ad65e69001bb296d/package.json#L10)
• Runs a test against both the original (json.js) and it's minified (json.pack.js) variant.

The test shows that the outputs are not the same.

The issue is in
https://github.com/aeschli/sample-terser-bug/blob/662548260936142a48e9c0d2ad65e69001bb296d/json.pack.js#L75

The switch statement also does an assignment to n.

• This corrupts the program as n is already used for the length of the input string of the scanner.
• The assignment to n is unnecessary

The corresponding line is
https://github.com/aeschli/sample-terser-bug/blob/662548260936142a48e9c0d2ad65e69001bb296d/json.js#L117

A workaround is to set hoist_funs to true

[aeschli]

What also fixes the issue is to avoid the re-declaration of ch at https://github.com/aeschli/sample-terser-bug/blob/662548260936142a48e9c0d2ad65e69001bb296d/json.js#L143
renaming ch to ch2 .

[fabiosantoscode]

Hey there!

Thank you for your report. Could you try to come up with a minimal reproduction of the bug? A whole repository is a bit big to find bugs inside, and it's probably possible to reproduce this in a minimal way.

[aeschli]

I tried, but it's very difficult as any change (e.g. simplification) resulted in a different result on the minifier.

I suspect it has to do with the variable shadowing. const ch is is defined in an outer and an inner scope. https://github.com/terser/terser/issues/335 is a similar issue.

[fabiosantoscode]

This is probably unrelated to that issue since you can only reproduce that with a catch block specifically, which has a tricky implementation due to historical reasons (old Uglify code is var-centric and the catch block binding is the only block scoped variable in oldjs, and to make that trickier there's a nasty IE8 bug which changes scoping rules for that binding)

If simple shadowed variables were the issue it would've been fixed years ago.

[fabiosantoscode]

I've managed to remove most of the input code and create a single JS file which is executable and outputs either PASS or FAIL, which is what we use in most of our tests:

const leak = () => null                                    

function createScanner(text) {                             
    let pos = 0;                                           
    let len = text.length;                                 
    function scanString() {                                
        while (true) {                                     
            if (pos >= len) {                              
                leak(text)                                 
                break;                                     
            }                                              
            let ch = text.charCodeAt(pos++);               
            if (ch === 92 /* backslash */) {               
                ch = text.charCodeAt(pos++);               
                switch (ch) {                              
                    case 34 /* doubleQuote */:             
                        break;                             
                    case "never-reached":                  
                        const ch = Math.random();          
                        leak(ch)                           
                        break;                             
                    default:                               
                        throw 'err'                        
                }                                          
                continue;                                  
            }                                              
            pos++;                                         
        }                                                  
    }                                                      
    function scanNext() {                                  
        if (pos >= len) return 17                          
        let code = text.charCodeAt(pos);                   
        pos++                                              
        scanString()                                       
        return 10                                          
    }                                                      
    return [() => pos, scanNext]                           
}                                                          

const [scan, getPos] = createScanner('"\\"')               
scan()                                                     
scan()                                                     

console.log(getPos() === 3 ? 'PASS' : 'FAIL')              

Eventually the purpose of creating the perfectly minimal reproduction of an error is to use that directly in the compress tests. However this is still a tad big, I'm thinking I can make this smaller still by ditching some of the state in it.

[fabiosantoscode]

I got this further down. I think I'm ready to devise a fix.

const leak = () => null                           

function scan() {                                 
    let len = leak('something');                  
    let ch = 0;                                   
    ch = 123;                                     
    switch (ch) {                                 
        case "never-reached":                     
            const ch = leak();                    
            leak(ch);                             
    }                                             
    return len === 123 ? 'FAIL' : 'PASS'          
}                                                 

console.log(scan())