Closed alexlamsl closed 7 years ago
May be related, bigger brother:
// original code
// (beautified)
var a = 100, b = 10, c = 0;
{
!function f0(bar) {
+function decodeURI() {
}();
if (a++ + --b) {
for (var brake5 = 5; (c = c + 1) + (--b + a++ || a || 3).toString() && brake5 > 0; --brake5) {
try {
switch (--b + /[abc4]/.test((--b + (b ^= a) || b || 5).toString())) {
case (c = c + 1) + --b:
try {
{
c = 1 + c, "number" % "" == 24..toString() || -3 - 24..toString() == 5 & "undefined" <= NaN;
c = 1 + c, "foo" <= -2 % 0 !== -4 ^ "undefined" + -4 < 2 ^ null;
}
} catch (parseFloat_12) {
{
}
c = c + 1;
}
{
var brake15 = 5;
while ((--b + 1 === 1 ? a : b) && --brake15 > 0) {
!function f1(Infinity_17) {
}((c = 1 + c, 1 ^ 23..toString() >> null ^ 22, 25 ^ 24..toString() === -2 && 22));
}
}
break;
case a++ + {
"\t": a++ + [ (c = 1 + c, "number" | -4 | "object" === "bar" <= [] === 0, -1 == NaN) ].Infinity,
b: typeof encodeURIComponent_18 != "number",
"\t": {
length: (c = 1 + c, c = c + 1, (a = -5 + 25) << {} >> 23..toString()),
in: (c = 1 + c, -5 >>> 22 - 3 | "foo" && -5 ^ -1 || -3 ^ [ , 0 ][1]),
3: (c = 1 + c, (a &= 23..toString() * -1 >= (a = 38..toString() & NaN)) != [ , 0 ][1] == -0 << undefined == undefined)
}.a
}[(c = c + 1) + typeof --b]:
{
var brake19 = 5;
while (--b + false && --brake19 > 0) {}
}
var isFinite_22 = --b + 0 === 1 ? a : b, parseFloat = --b + [ (c = 1 + c, "function" ^ 1,
(isFinite_22 = 22 - Infinity) >= 23..toString() * "function" && 23..toString() * "bar"), , (c = 1 + c,
-2 ^ 5 << "" << Infinity >> 0 % -5 <= ([ , 0 ].length === 2) >>> "undefined") ];
break;
case a++ + ++a:
--b + [ , (c = 1 + c, isFinite_22 != -3 !== Infinity >>> 0 << 25 <= false >= 22 <= 24..toString() << "undefined") ][(c = c + 1) + 1 === 1 ? a : b];
break;
case a++ + (--b + {
0: --b + [ (c = 1 + c, -3 >> "foo" >= null >> "undefined" ^ 3 > "foo" || "object",
"undefined"), , (c = 1 + c, 3 << ([ , 0 ].length === 2) === Infinity ^ 23..toString() || -3 * NaN ^ 1 - "bar"), (c = 1 + c,
1 / 1, true & 5, -5 * 24..toString(), "foo" !== 3), (c = 1 + c, "object" % /[a2][^e]+$/ ^ 1 ^ 3 && 3 + -4 >> 4 + ([ , 0 ].length === 2)) ].length,
undefined: a++ + (c = 1 + c, -4 && null | 3 && -4 << (isFinite_22 *= 1 / "") <= [ , 0 ][1] * -1),
b: a++ + ([ , 0 ].length === 2) ^ 22 === [ , 0 ][1] & -2 + (c = c + 1, "foo" === "foo")
} || a || 3).toString():
break;
}
} catch (encodeURIComponent_24) {
{
var brake25 = 5;
while (--b + 38..toString() && --brake25 > 0) {
!function f2(a) {
c = 1 + c, [] >>> 38..toString() + (encodeURIComponent_24 = 1 >= -1), /[a2][^e]+$/ * NaN || "function" !== [];
c = 1 + c, [] / NaN !== -2 > 4 < "object", undefined * "" + "function";
}(--b);
}
}
"undefined";
} finally {
try {
{
var a_33;
{
}
{
}
}
} finally {
}
c = c + 1;
}
}
} else {
var brake39 = 5;
do {
c = c + 1;
} while (((c = c + 1) + -1 && -0 === "foo", undefined >= ([ , 0 ].length === 2) & null,
undefined ^ "foo") && --brake39 > 0);
}
}(0 === 1 ? a : b);
}
var NaN = b = a, a = (c = c + 1) + -0 ^ "number" ^ 5 / 3 + 3 ^ 23..toString() == 1 >>> 24..toString() && {
1.5: a++ + typeof decodeURI_42 == "crap",
Infinity: /[abc4]/.test(((a++ + (c = c + 1, undefined) || 1 >= 2 == "undefined",
-1 >> 3 | -0 / --b + [ , /[abc4]/.test((typeof encodeURIComponent === "symbol" || b || 5).toString()), --b + -a, --b, a++ + 0 === 1 ? a : b ]) || b || 5).toString()),
foo: +function() {}(),
null: --b + a--,
c: b + 1 - .1 - .1 - .1
};
console.log(null, a, b, c);
// uglified code
// (beautified)
var a = 100, b = 10, c = 0;
!function(t) {
if (a++ + --b) {
for (var n = 5; (c += 1) + (--b + a++ || a || 3).toString() && n > 0; --n) {
try {
switch (--b + /[abc4]/.test((--b + (b ^= a) || b || 5).toString())) {
case (c += 1) + --b:
try {
c = 1 + c, 0 / 0 == 24..toString() || 24..toString(), c = 1 + c;
} catch (t) {
c += 1;
}
for (var o = 5; (1 + --b == 1 ? a : b) && --o > 0; ) {
c = 1 + c, 23..toString(), 24..toString();
}
break;
case a++ + {
"\t": a++ + [ (c = 1 + c, -1 == NaN) ].Infinity,
a: "number" != typeof encodeURIComponent_18,
"\t": {
length: (c = 1 + c, c += 1, (a = 20) << {} >> 23..toString()),
in: (c = 1 + c, 4),
3: (c = 1 + c, (a &= -1 * 23..toString() >= (a = 38..toString() & NaN)) != [ , 0 ][1] == 0 == void 0)
}.b
}[(c += 1) + typeof --b]:
for (var e = 5; --b + !1 && --e > 0; ) {}
var r = 0 + --b == 1 ? a : b;
--b, c = 1 + c, (r = -1 / 0) >= "function" * 23..toString() && 23..toString(), c = 1 + c,
[ , 0 ].length;
break;
case a++ + ++a:
--b, [ , (c = 1 + c, -3 != r != !1 <= 24..toString() << "undefined") ][(c += 1) + 1 === 1 ? a : b];
break;
case a++ + (--b + {
0: --b + [ (c = 1 + c, "undefined"), , (c = 1 + c, 3 << (2 === [ , 0 ].length) == 1 / 0 ^ 23..toString() || -3 * NaN ^ 0 / 0), (c = 1 + c,
24..toString(), !0), (c = 1 + c, -1 >> 4 + (2 === [ , 0 ].length)) ].length,
undefined: a++ + (c = 1 + c, -4 << (r *= 1 / 0) <= -1 * [ , 0 ][1]),
a: a++ + (2 === [ , 0 ].length) ^ 22 === [ , 0 ][1] & -2 + (c += 1, !0)
} || a || 3).toString():
}
} catch (t) {
for (var i = 5; --b + 38..toString() && --i > 0; ) {
!function(n) {
c = 1 + c, 38..toString(), t = !0, c = 1 + c;
}(--b);
}
} finally {
try {} finally {}
c += 1;
}
}
} else {
do {
c += 1;
} while (c += 1, [ , 0 ].length, 0);
}
}();
var NaN = b = a, a = (c += 1) + -0 ^ "number" ^ 5 / 3 + 3 ^ 23..toString() == 1 >>> 24..toString() && {
1.5: a++ + typeof decodeURI_42 == "crap",
Infinity: /[abc4]/.test((a++, c += 1, -1 | -0 / --b + [ , /[abc4]/.test(("symbol" == typeof encodeURIComponent || b || 5).toString()), --b - a, --b, 0 + a++ == 1 ? a : b ] || b || 5).toString()),
c: 0 / 0,
null: --b + a--,
d: b + 1 - .1 - .1 - .1
};
console.log(null, a, b, c);
original result:
null { '1.5': false,
Infinity: false,
foo: NaN,
null: 11,
c: 2.6999999999999997 } 2 78
uglified result:
null { '1.5': false,
Infinity: false,
c: NaN,
null: 11,
d: 2.6999999999999997 } 2 78
minify(options):
{ compress: { warnings: false },
mangleProperties: {},
fromString: true }
Three of a kind?
// original code
// (beautified)
var a = 100, b = 10, c = 0;
var a = {
foo: {
c: (c = c + 1) + (--b + Infinity || a || 3).toString(),
length: --b + /[abc4]/.test(((c = c + 1) + (b-- || 6).toString()[(c = c + 1) + (a++ + [ --b, (c = c + 1) + Infinity === NaN >= "" == [] % "foo" * -4 | 0 === "number" >> (c = 1 + c,
Infinity % Infinity >> NaN % 22 === 1 < NaN | {} <= false), {}[(c = 1 + c, null % {} + {} === 25 / -0 & "undefined" / -3 - 2)], (c = c + 1) + b + 1 - .1 - .1 - .1, function() {
}() ].length)] || b || 5).toString()),
c: a--
},
in: -5,
foo: (--b + {
in: {}.length,
foo: (c = c + 1) + typeof encodeURI_2 == "number"
} || a || 3).toString()
};
c = c + 1;
console.log(null, a, b, c);
// uglified code
// (beautified)
var a = 100, b = 10, c = 0, a = {
a: {
b: (c += 1) + (--b + 1 / 0 || a || 3).toString(),
length: --b + /[abc4]/.test(((c += 1) + (b-- || 6).toString()[(c += 1) + (a++ + [ --b, (c += 1) + 1 / 0 === !1 == [] % "foo" * -4 | 0 == "number" >> (c = 1 + c,
!1 | {} <= !1), {}[(c = 1 + c, null % {} + {} === -1 / 0 & NaN)], (c += 1) + b + 1 - .1 - .1 - .1, void 0 ].length)] || b || 5).toString()),
b: a--
},
in: -5,
a: (--b + {
in: {}.length,
a: (c += 1) + typeof encodeURI_2 == "number"
} || a || 3).toString()
};
c += 1, console.log(null, a, b, c);
original result:
null { foo: '5[object Object]', in: -5 } 5 9
uglified result:
null { a: '5[object Object]', in: -5 } 5 9
minify(options):
{ compress: { warnings: false },
mangleProperties: {},
fromString: true }
// original code
// (beautified)
var a = 100, b = 10, c = 0;
{
{}
var a = {
in: --b + new function() {
a++ + (a++ || a || 3).toString();
return Math;
}() ? (c = c + 1) + {} ? a++ + (--b + a--) : --b + a++ : (new function() {
return Math;
}() || 4).toString()[a++ + ~a],
b: (--b + 23..toString(), [] ^ "" == 4 < NaN * null >>> NaN + -4)
};
}
{
!function f0(eval) {
{
!function f1(undefined_6) {
try {
new function() {
return Math;
}();
} finally {
var isFinite_12 = b = a, Infinity_13 = new function() {
((c = 1 + c, [] * 0 < true === true >= "foo", "", 22 & 0) || 1).toString()[(c = 1 + c,
"function" >= "function" <= -2 % 25 & (c = c + 1, [ , 0 ][1]) / NaN === 25)];
return Math;
}();
}
}("undefined" < 24..toString() < -1, [] ^ [] << true << [] != -0 / (c = c + 1) + 23..toString() * null + -4 + 38..toString() % (c = c + 1,
24..toString() <= null));
}
var isFinite_15 = b >>= a;
}(1 === 1 ? a : b);
}
console.log(null, a, b, c);
// uglified code
// (beautified)
var a = 100, b = 10, c = 0, a = {
in: --b + new function() {
return a++, (a++ || a || 3).toString(), Math;
}() ? (c += 1) + {} ? a++ + (--b + a--) : --b + a++ : (new function() {
return Math;
}() || 4).toString()[a++ + ~a],
a: (--b, 23..toString(), !0 ^ [])
};
!function(n) {
!function(n) {
try {
new function() {
Math;
}();
} finally {
b = a, new function() {
(c = 1 + c, 1).toString()[(c = 1 + c, !1 & 25 == (c += 1, [ , 0 ][1] / NaN))], Math;
}();
}
}(24..toString(), (c += 1, 23..toString(), 38..toString(), c += 1, 24..toString()));
b >>= a;
}(), console.log(null, a, b, c);
original result:
null { in: 213, b: 1 } 0 6
uglified result:
null { in: 213, a: 1 } 0 6
minify(options):
{ compress: { warnings: false },
mangleProperties: {},
fromString: true }
I was wondering when we'd get a case where an object literal would be output in the result - and here we go - array and object fuzzing is working nicely.
These cases are all likely correct. When a property is mangled, the properties in an object literal result will naturally be different compared to the original result.
One way to verify that would be to generate a name cache for each test case and see that the property mangling mappings are correct. Another way would be to impose a pre-built name cache upon the mangle-props tests so that the dozen or so pre-chosen property names' mappings would be the same in every case. A third way would be to generate debug property name mappings so they are very easy to visually inspect without a name cache. An example of debug prop mangling for the first original test case:
bin/uglifyjs --mangle-props --mangle-props-debug -b | node
null 104 { in: 9, var: '23', '_$b$_': '81103,NaN,103,' } 5
as compared to the original result:
null 104 { in: 9, var: '23', b: '81103,NaN,103,' } 5
which we can see is correct. This check can be automated by stripping off "_$"
and "$_"
from the console.log
result string in the mangleProperties tests specifically, as these substrings are not likely to be output in tests.
However, there is one thing I noticed that I did not like in these cases - the properties in
, null
and var
were not mangled. This seems to be a regression from 2.7.5. There's no reason why I can think of why properties that happen to be keywords cannot be mangled. Would have to bisect the commits to see when and why this was introduced - whether intentionally or by accident.
The keyword properties regression was introduced in eb55d8a9bb37cc28303ace91337784dbf0777d03:
--- a/lib/propmangle.js
+++ b/lib/propmangle.js
@@ -149,7 +149,6 @@ function mangle_properties(ast, options) {
// only function declarations after this line
function can_mangle(name) {
+ if (!is_identifier(name)) return false;
if (unmangleable.indexOf(name) >= 0) return false;
if (reserved.indexOf(name) >= 0) return false;
if (options.only_cache) {
When the first original example is run without this commit we can see the desired effect:
$ bin/uglifyjs --mangle-props --mangle-props-debug -b | node
null 104 { '_$in$_': 9, '_$var$_': '23', '_$b$_': '81103,NaN,103,' } 5
as compared to the original result:
null 104 { in: 9, var: '23', b: '81103,NaN,103,' } 5
I recommend that eb55d8a9bb37cc28303ace91337784dbf0777d03 be reverted. Since #1481 was looking to overcome a bug in IE8, less optimally we could add an additional screw_ie8 == false
check be added to the condition in the original commit. But the author of the PR acknowledged that had the correct uglify flags been used in the first place there would not have been a need to for the PR in question.
(I'm out of my depth with mangleProperties
, so apologies in advance if what I said below doesn't make sense.)
Pre-populated name cache or debug flag would not generate naming collision when compared to the default behaviour, wouldn't it? If so, then we won't be able to detect bugs on the same level as mangle
?
However, there is one thing I noticed that I did not like in these cases - the properties in , null and var were not mangled.
AFAICT mangleProperties
are all self-contained within lib/propmangle.js
, so I guess the breakage would be confined to that file.
Give me a shout, preferrably alongside a test/compress/
test case, if you want a hand in tracking down lines of code. Otherwise I'll be taking a seat at the back. :popcorn::sunglasses:
The keyword properties regression was introduced in eb55d8a :
Ah, that commit. I remember it well...
Do you mind giving me a test case for this? That way I can:
screw_ie8
specific behaviour is necessaryHmm, this is weird - removing that line causes "-Infinity"
to be mangled, which in turn causes the test to fail (even with the mangled name to match expect
, it would then fail at expect_stdout
).
So I think "-Infinity"
and friends (say "~undefined"
) is a different bug, which #1481 happens to mask.
Let me grab a shower and think over what the correct fix may entail.
Pre-populated name cache or debug flag would not generate naming collision when compared to the default behaviour, wouldn't it?
No, it would not generate a collision. But for fuzz testing I think the --mangle-props-debug
route is better.
Test case shown using uglify-js@2.7.5
:
$ echo 'var o = {foo: 1, var: 2}; console.log(o.foo, o.var);' | uglifyjs --mangle-props
var o={a:1,b:2};console.log(o.a,o.b);
I don't know why the PR author wanted to keep keyword properties not mangled, but to achieve that effect you'd either have to quote the properties in question:
$ echo 'var o = {foo: 1, "var": 2}; console.log(o.foo, o.var);' | uglifyjs --mangle-props=unquoted --support-ie8 -c
var o={a:1,"var":2};console.log(o.a,o["var"]);
or do this:
$ cat reserved.json
{
"vars" : [],
"props": [ "do", "while", "var", "for" ]
}
$ echo 'var o = {foo: 1, var: 2}; console.log(o.foo, o.var);' | uglifyjs --mangle-props --reserved-file reserved.json --support-ie8 -c
var o={a:1,"var":2};console.b(o.a,o["var"]);
or even this:
$ echo 'var o = {foo: 1, var: 2}; console.log(o.foo, o.var);' | uglifyjs --mangle-props --mangle-regex '/^(?!(?:var|do|while|for)$).*$/' --support-ie8 -c
var o={a:1,"var":2};console.log(o.a,o["var"]);
Thanks for the clarifications. I can now remember some of the (claimed) reasons behind that PR.
IIRC, the aim was trying to avoid mangled names to end up looking like a keyword. As the name generator counts like this:
a
, b
, c`, ...aa
, ba
, ...It is theoretically possible to generate things like if
or in
or even for
. But obviously the code changes in that PR won't achieve the stated goal.
It is theoretically possible to generate things like if or in or even for. But obviously the code changes in that PR won't achieve the stated goal.
I see. Simply using --support-ie8
would suffice then.
Now for a separate issue (which #1481 conveniently masked for this particular corner case):
$ echo 'console.log(({"-Infinity":1})[-1/0])' | bin/uglifyjs --mangle-props
console.log({a:1}[-1/0]);
$ echo 'console.log(({"-Infinity":1})[-1/0])' | node
1
$ echo 'console.log(({"-Infinity":1})[-1/0])' | bin/uglifyjs --mangle-props | node
undefined
Please advise on how we shall proceed?
The "-Infinity" property thing is puzzling. I'm open to any suggestions.
I'm willing to add them to reserved
if there are limited cases of these. I know that things like "+NaN"
can't happen, and off the top of my head I can't immediately think of more cases than -Infinity
.
The "-Infinity"
property does seem to be a one-of-a-kind situation. Can't do anything similar with undefined
and NaN
as far as I know.
-0
? :smiling_imp:
Ah, of course:
$ uglifyjs -V
uglify-js 2.7.5
$ echo 'console.log(({"-0":1})[-0])' | uglifyjs --mangle-props | node
undefined
So just 2 special cases, then?
Let's settle on those two for now. If something else pops up, we'll come up with a better fix.
It's how software development works anyway, I think. :roll_eyes:
Javascript certainly has some odd corner cases. Likely most are artifacts from the initial implementation that have to be grandfathered.
Turns out you can't access -0
as a string:
$ node
> "" + -0
'0'
> "" + -""
'0'
So I think we are good with just -Infinity
as the special case. I'll add -0
to the test case just to make sure.
@alexlamsl Can you please add that "-0"
prop test to issue-1770.js
? Also could you add just one normal property that can be mangled to that test case just to show it is indeed working?
Can you please add that "-0" prop test to issue-1770.js ?
Great minds think alike :tm:
Also could you please set expect_stdout
in issue-1770.js
to the actual output string?
@alexlamsl Which approach do you want to use for mangleProperties fuzz testing? Just enable the props debug option (whatever that is in minify
)?
That I haven't given much thought yet - and I do need that shower brb
mangle props debug option usage:
mangle_debug: {
mangle_props = {
debug: ""
};
input: {
a.foo = "bar";
x = { baz: "ban" };
}
expect: {
a._$foo$_ = "bar";
x = { _$baz$_: "ban" };
}
}
@alexlamsl How many fuzz iterations were run to generate a few results with object literals?
@alexlamsl You could add a custom debug mangle string, but the default one should be adequate:
--- a/test/ufuzz.json
+++ b/test/ufuzz.json
@@ -17,7 +17,9 @@
"compress": {
"warnings": false
},
- "mangleProperties": {}
+ "mangleProperties": {
+ "debug": ""
+ }
},
{
"compress": {
@@ -48,7 +50,8 @@
"toplevel": true
},
"mangleProperties": {
- "ignore_quoted": true
+ "ignore_quoted": true,
+ "debug": ""
},
"output": {
"keep_quoted_props": true
How many fuzz iterations were run to generate a few results with object literals?
Lowest was around ~0.5 kFuzz, while others are around several kFuzz.
So my hypothetical scenario goes like this:
var o = {
a: 1,
foo: 2,
};
// prints "1"
console.log(o.a);
If a new bug is introduced in mangleProperties
like this:
var o = {
a: 1,
a: 2,
};
// prints "2"
console.log(o.a);
--mangle-props-debug
would mask that and won't cause any test failures:
var o = {
_$a$_: 1,
_$foo$_: 2,
};
// prints "1"
console.log(o._$a$_);
I don't think we should be worried about hypothetical failures.
If such a fundamental prop collision bug happened in the wild it would render the feature completely useless and it would have been reported.
If such a bug fundamental prop collision bug happened in the wild it would render the feature completely useless and it have been reported.
I hope you said that in earnest...
Jokes aside, point taken. I'll spin up a PR with your suggest shortly.
(Disclaimer: I am not pulling a british by commenting on your grammar :ghost:)
I should grammar check my posts before I submit them. That's what happens when I edit a sentence. Better to delete the whole thing and try again.
What do you think of this?
--- a/test/ufuzz.js
+++ b/test/ufuzz.js
@@ -825,6 +825,8 @@ for (var round = 1; round <= num_iterations; round++) {
if (ok) {
original_result = sandbox.run_code(original_code);
uglify_result = sandbox.run_code(uglify_code);
+ if (options.mangleProperties && options.mangleProperties.debug !== undefined)
+ uglify_result = uglify_result.replace(/_\$|\$_/gm, "");
ok = sandbox.same_stdout(original_result, uglify_result);
}
if (verbose || (verbose_interval && !(round % INTERVAL_COUNT)) || !ok) log(options);
Edit: this patch does not work.
I have a more elaborate solution and am about to click the create PR button.
Short & sweet