Uglifyjs: unsafe option inlines values from json object

[jbedard]

Bug report or feature request? bug

ES5 or ES6+ input? es5

uglify-js version reproduced in 2.8.14 and 2.8.22, works in 2.7.5

JavaScript input

function foo(x) { x.b=false; } console.log(function m(){var a={b:true}; foo(a); return a.b; }())

The uglifyjs CLI command executed
uglify -c unsafe

JavaScript output produced
function foo(x){x.b=!1}console.log(function(){var a={b:!0};return foo(a),!0}());

Note the return true at the end instead of return a.b

The documentation for unsafe doesn't mention any such modifications, so I'm not sure why this is triggered from that specific option. Either way I don't think such a value should ever be inlined when the parent object is passed into a function?

[alexlamsl]

This is working as expected for unsafe - PR to update documentation is welcomed.

[kzc]

$ node_modules/.bin/uglifyjs -V
uglify-js 2.8.22

$ echo 'function foo(x) { x.b=false; } console.log(function m(){var a={b:true}; foo(a); return a.b; }())' | node
false



md5-491cf0e46e4d48d373763140b67fed52



$ echo 'function foo(x) { x.b=false; } console.log(function m(){var a={b:true}; foo(a); return a.b; }())' | node_modules/.bin/uglifyjs -c | node
false



md5-491cf0e46e4d48d373763140b67fed52



$ echo 'function foo(x) { x.b=false; } console.log(function m(){var a={b:true}; foo(a); return a.b; }())' | node_modules/.bin/uglifyjs -c unsafe | node
true

It's a bug. foo(a) modifies a and a.b is incorrectly replaced by true by reduce_vars.

[jbedard]

Really? :(

unsafe says it "enables some transformations that might break code logic in certain contrived cases, but should be fine for most code". Is a method modifying a JSON object a "contrived case"...?

Could this be put behind a different unsafe_* flag?

EDIT: what @kzc says makes sense

[alexlamsl]

@kzc foo(a) is reading from a, so when it has a constant value, say when a=42, it is completely safe to not have to consider any modifications from foo(). Objects (or arrays) aren't constant values, which is why this particular section of evaluate & reduce_vars is gated by unsafe.

@jbedard just to clarify, object literal is not JSON

[kzc]

it is completely safe to not have to consider any modifications from foo()

@alexlamsl That's clearly incorrect.

-c unsafe produces an incorrect result as compared to unmodified source code when run on node. Please see my last post.

[alexlamsl]

@kzc I thought unsafe is supposed to generate incorrect output for some cases?

I understand why and how it breaks in this particular case - I'm just not sure if unsafe definitely needs to handle that.

[kzc]

I thought unsafe is supposed to generate incorrect output for some cases?

No, never intentionally.

unsafe can make assumptions about Array.prototype.join not being replaced by the user, for example:

$ echo 'Array.prototype.join = function(){}; console.log([1, "B", 3].join());' | node
undefined

$ echo 'Array.prototype.join = function(){}; console.log([1, "B", 3].join());' | node_modules/.bin/uglifyjs -c unsafe | node
1,B,3

But unsafe should not produce an incorrect result for regular sane javascript code.

[alexlamsl]

@kzc understood - will fix.

[jbedard]

If unsafe generates "incorrect output for some cases" then no one would ever use it. As I understand it should work fine for non-sketchy/sane javascript code (no overriding join, no sketchy things like toString/getters with side effects etc.)

[kzc]

@alexlamsl Sorry for coming out of retirement on this issue. :-)

Some old habits die hard.

[alexlamsl]

@kzc I hope my eventual retirement would last a bit longer :wink:

Jokes aside - I welcome any help I can get from you, so keep them coming whenever you see fit to chip in.

[kzc]

I was wondering why unsafe was removed from test/ufuzz.json. There's no reason I can think of why it shouldn't be fuzzed.

[jbedard]

I still haven't been able to turn unsafe back on due to either this issue or something very similar. I think the fix didn't fix all cases.

In my case the js is similar to:

var options = {
    inputAndOutput: true
};
return methodModifyingTheOptions(..., {data: {options: options}}).then(function(result) {
    if (result === 'something') {
        return Promise.reject();
    }
    return options.inputAndOutput;
})

The options.inputAndOutput still gets replaced with true even though it is passed into a method.

The only thing I can think of is that the object literal is nested within another literal when passed to the method, and maybe the existing fix doesn't catch that case?

[alexlamsl]

@jbedard I tried your example above and can't seem to reproduce the issue:

$ uglifyjs -V
uglify-js 3.3.12

$ cat test.js
function f() {
    var options = {
        inputAndOutput: true
    };
    return methodModifyingTheOptions({data: {options: options}}).then(function(result) {
        if (result === 'something') {
            return Promise.reject();
        }
        return options.inputAndOutput;
    });
}

$ uglifyjs test.js -bmc unsafe
function f() {
    var n = {
        inputAndOutput: !0
    };
    return methodModifyingTheOptions({
        data: {
            options: n
        }
    }).then(function(t) {
        return "something" === t ? Promise.reject() : n.inputAndOutput;
    });
}

[jbedard]

Hmmm. I guess I didn't actually verify that snippet and was just trying to mimic something. I'm starting to think webpack is not using the version of uglify that I thought it was, so maybe this is just my own configuration problem.

Sorry for the trouble, I'll post back here if I figure it out...

[alexlamsl]

@jbedard no worries and thanks for taking the time :wink:

[jbedard]

Yeah the issue was just that the default uglifyjs shipped with webpack (pre-v4) is uglify v2, and I assume this fix was only put into v3. Seme info at the top of the webpack-uglify page: https://webpack.js.org/plugins/uglifyjs-webpack-plugin/

[alexlamsl]

I can confirm the fix first appears in [email protected]