[ES6] Bug: undefined _is_ a function, sometimes

[pvdz]

• Bug report or feature request?
  Bug: Found by fuzzer

• uglify-js version (uglifyjs -V)
  uglify-js 2.8.15 (git master)

var a = 100, b = 10;
function f() {
  for (var v=1; undefined && v; --v){
    a--
  }
  {
    function undefined() {}
    undefined();
  }
  var NaN725 = b = a
}
f();

console.log(a, b);

$ node s.js && bin/uglifyjs s.js -c | node
100 100
WARN: Dropping side-effect-free statement [s.js:8,4]
WARN: Side effects in initialization of unused variable NaN725 [s.js:10,6]
99 99

function f(){function undefined(){}for(var v=1;undefined&&v;--v)a--;b=a}var a=100,b=10;f(),console.log(a,b);

except it wasn't. As usual, this is a little weird. The block around the undefined function (hey, turns out undefined is a function!) is required for the bug to show up. Not sure why since there is no real block scoping in ES5, but perhaps this is an obscure function declaration edge case and perhaps even only in nodejs.

I think undefined is incorrectly assumed to be the global actual undefined value in this weird edge case?

[kzc]

There's no end of trouble when undefined is redefined. The rules of physics break down at that point. Would have to test it on a few browsers to see if there's consensus.

[kzc]

If you're going to redefine undefined then you might as well redefine arguments and Infinity as well.

[pvdz]

shrug I don't think the particular case is important. But there could be an underlying case that is important. I can't judge on that since Uglify is a black box for me.

Oh arguments is a good one :) NaN and Infinity are already a possible var/func name in the fuzzer. Not leading to too many bugs. Heck, neither is undefined apparently.

[pvdz]

(btw, arguable, valid use cases are an obfuscator or sandbox breaker...)

[kzc]

@qfox Don't mind my complaining about Javascript. It's a quirky language. Please keep up the good work breaking Uglify in the most obscure ways possible.

[kzc]

Nested inner functions would be a fruitful area of fuzzing I'm sure.

[alexlamsl]

So is this a duplicate of #1666?

[alexlamsl]

To answer my own question - even if this is not a duplicate, this certainly only affects ES6:

> nvs use node
PATH = node\7.7.3\x64

> node test.js
100 100

> node bin\uglifyjs test.js -c | node
WARN: Dropping side-effect-free statement [test.js:8,4]
WARN: Side effects in initialization of unused variable NaN725 [test.js:10,6]
99 99

> nvs use node/0.12
PATH = node\0.12.18\x64

> node test.js
99 99

> node bin\uglifyjs test.js -c | node
WARN: Dropping side-effect-free statement [test.js:8,4]
WARN: Side effects in initialization of unused variable NaN725 [test.js:10,6]
99 99

[pvdz]

Hmmm so I'm not sure here. Should the UglifyJS ES5 branch result in code that only takes "es5" runtimes into account, or to code that works in the most current environments. Because if it's the latter than this kind of bug shouldn't be overlooked. And honestly, I think it's the expectation that the resulting code just works, even if the input was, strictly speaken, ES5.

And so in that case this shouldn't be pushed to be fixed on an ES6 branch but right here. That's just my two cents on it.

[pvdz]

I think this is a simplified test case of the same problem:

var x = 0;
{
  function x() {
    console.log('PASS')
  }

  x();
}

Note that it wasn't that obvious when considering the fuzzer spit out something like this:

var a = 100, b = 10;
function f52663() {
  var x = 0;
  for (var brake401755 = 1; brake401755 > 0; --brake401755) {
    if (1) {
      function x() {
        console.log('PASS')
      }

      x();
    }
  }
}

f52663();

console.log(a, b);

(Point is that it doesn't necessarily need to be a redundant plain block, it could be part of a sub-statement)

[alexlamsl]

IMHO it is a behaviour which breaks backward compatibility, since no new keywords are introduced to distinguish this unlike let (though they broke const in that regard, IIRC).

And given that ES6 itself is still largely a work in progress, I'm none too keen to work on it myself. That being said, I won't opposed to any PRs which present minimal changes that would allow master to workaround such quirks.

[kzc]

Should the UglifyJS ES5 branch result in code that only takes "es5" runtimes into account, or to code that works in the most current environments.

The code on master only handles ES5 and the code above is ES6. Uglify on master could issue a warning or error for declaring functions in blocks similar to the error node 0.10.x and 0.12.x produce.

[pvdz]

That's debatable. The code is technically not valid es5 but it has always worked to some degree in all envs due to backwards compat. I would assume all code from the es5 era is targeted here. At the same time I don't really know how often you'd find these cases in the wild for similar reasons.

…

On 25 Mar 2017 2:58 pm, "kzc" ***@***.***> wrote: Should the UglifyJS ES5 branch result in code that only takes "es5" runtimes into account, or to code that works in the most current environments. The code on master only handles ES5 and the code above is ES6. Uglify on master could issue a warning or error for declaring functions in blocks similar to the error node 0.10.x and 0.12.x produce. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1664 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMzmXklNGAx6Stx-QFttec_Xczjh9kyks5rpR1ygaJpZM4Mo2Tg> .

[kzc]

The code is technically not valid es5 but it has always
worked to some degree in all envs due to backwards compat.

Uglify on master only handles ES5. In ES5 there is no concept of block scope - only function scope. What ECMAScript has essentially done was to take invalid ES5 code and make it valid ES6 code. Since Uglify is ES5 only it cannot possibly handle this scenario except to issue an error, or take garbage in and produce garbage out.

[pvdz]

As a consume I expect Uglify to take my code, ES5 in this case, and produce code that'll work in the browser and in node. Agreed that both envs make a total mess out of it with the advent of ES6, but that is reality and allowing minification tricks that would cause actual syntax errors to occur is detrimental to the whole stack.

The consumer of this library will not accept an argument "but blocks are es6" when the input was ES5 and worked before minification. You'll be hard pressed to find any consumer that actually only wants to run the result of uglify in an actual ES5-only env. In fact, I don't think you'll find any at all. So unless this project turned into a theoretical tool, that goal seems ... bad.

As a tooling dev my rewrites tend to have lots of blocks as they are the only way to "group" multiple statements into one. That makes them safe for transforms without risking breaking a sub-statement (like if and loops) that wasn't already wrapped in a block. This "trick", if you want to call it a trick, is also used in the fuzzer a few times for the same reason. My point is that those artifacts that the fuzzer creates could easily be created by transformation tools. So the problem seems real to me, unless it only affects undefined (/NaN/Infinity) because redeclaring those is a super edge case.

It's obviously not up to me and I'll let it rest after this. But please consider that uglify should produce code that runs in current browsers and try not to let es6-or-not cloud your judgement whether or not a problem should be fixed. The end consumer of your library won't understand at all that it breaks when it does. Especially when it's very likely that the malfunction didn't even originate in their own code but somewhere in their tooling stack.

Meanwhile, I'll try to modify the fuzzer to circumvent these problems for the time being. Just like preventing function statements (the arguments against them are stronger imo).

[kzc]

@qfox Your argument is based on a false assumption. The test case in question is not ES5 compliant. It only superficially looks like ES5. It is really block scoped ES6/ES2015. Users wishing to minify block scoped ES6 should use uglify/harmony, Google Closure or Babel/Babili. uglify/master's AST and Compressor simply is not designed to handle ES6.

When users write invalid ES5 code and process it with uglify/master they are taking their chances and should have no expectation of any ES6 behavior. Uglify on master should technically reject invalid ES5 code with function declarations in blocks, but it is doing the users a favor by hoisting such non-ES5 conforming functions to make the code ES5 compliant.

If users really wanted to process such invalid ES5 with uglify/master and have it run within an ES6 browser environment they can disable compress and mangle. But in reality no one has ever reported such a problem in the wild with ES5 code with uglify/master.

[pvdz]

ok

[alexlamsl]

Just thinking out loud - I think the fix in uglify-es for this resides within can_be_evicted_from_block(), i.e. by adding AST_Defun to the list.