432077 - Crash [@ DecompileExpression] with trap, |with|, import

Status: RESOLVED WORKSFORME

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

function f(x) { with({}) with({}) { import x.y; } }
trap(f, 0, "");
f({});

Crash [@ DecompileExpression]

With one less "with", it gives a screwed-up error message instead of crashing:
"ReferenceError: import x.y;???x is not defined"

This happens both without and with the patch in bug 431465 comment 24.

Comment 1

[Brian Crowder]

There's definitely a UMR happening here (the ??? part of the decompilation),
even if I fix the null-dereference bug.  I'll attach a patch which fixes that
part shortly (I think the patch is still needed, even in light of Igor's work
in bug 430293).

Comment 2

[Brian Crowder]

Attachment: fixes NULL deref

This does not fix the whole bug...  perhaps combined with Igor's patch, it will, I'll try that next.

Comment 3

[Brian Crowder]

Comment on attachment 319430 [details] [diff] [review]
fixes NULL deref

This bug is fixed by the Igor's patch from bug 430293 comment #8.  My patch isn't needed.

Comment 4

[Jesse Ruderman]

|import| is no more.  WFM.

Comment 5

[Bob Clary [:bc] (inactive)]

shouldn't this be reopened and targeted to 1.9.0?

Comment 6

[Brian Crowder]

We should probably backport the import-removal.

Comment 7

[Mike Shaver (:shaver emeritus)]

Really?  We have a pretty bad recent record on content-affecting backports.  I would rather leave it for 3.1, so that we don't break people's sites inadvertently with a security update.

Comment 8

[Brian Crowder]

Aren't there a host of fuzzer-found (and other) |import| related bugs?  It seems that our import extension isn't used much (if at all), and that it increased attack-surface overall.  Do you know specific sites that use it?

Comment 9

[Mike Shaver (:shaver emeritus)]

No, I don't know of specific sites.  I didn't know of specific sites or extensions that used eval(o, s) either.

I am not arguing against the long-term removal of |import|, I'm arguing against taking the patch "because we have it", because the cost of continuing to break things with security updates is very high for us, in both short- (QA/build/etc.) and long-term (user inclination to promptly update).