593611 - Crash [@ js_regexp_toString]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(function() {
  __defineGetter__("x", /x/.constructor)
})()
for (var a = 0; a < 4; ++a) {
  if (a % 4 == 1) {
    gc()
  } else {
    print(x);
  }
}

crashes js opt shell on TM changeset 60af58b42567 without -m nor -j at js_regexp_toString.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

s-s because this involves gc.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   52720:66c8ad02543b
user:        Luke Wagner
date:        Mon Aug 16 12:35:04 2010 -0700
summary:     Bug 581263 - remove slow natives (r=waldo,mrbkap)

Comment 2

[Luke Wagner [:luke]]

Attachment: fix

Ouch, I missed a case where we assumed argv was initialized for [0..fun->nargs), so this was looking at trash stack values.  I wasn't able to repro the crash, but I was able to get a valgrind memcheck warning, which this patch fixes.

Comment 3

[Luke Wagner [:luke]]

The patch also puts back the "if (IsConstructing)" check, since 15.10.3.1 seems to require it.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 472433 [details] [diff] [review]
fix

Nasty -- so we want your C++ super-safe helper instead of the current unsafe FastNative signature, pretty soon.

>+        if ((argc >= 1 && argv[0].isObject() && argv[0].toObject().isRegExp()) &&

No need to over-paren these && terms against the final && at end of line.

>+            (argc == 1 || (argc == 2 && argv[1].isUndefined())))

We know argc >= 1 starting this line, so why the argc == 2 && bit?

Leaving r?waldo but wanted to give a quick r+ to get this in for further fuzzing.

/be

Comment 5

[Luke Wagner [:luke]]

(In reply to comment #4)
Thanks!

http://hg.mozilla.org/tracemonkey/rev/1d477685d70a

Comment 6

[Luke Wagner [:luke]]

D'oh!
http://hg.mozilla.org/tracemonkey/rev/9d7c084a9a56
regexp_construct is like a bermuda triangle for my brain.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 472433 [details] [diff] [review]
fix

My brain's gone from frazzled during the day to relaxed in the evening to sleepy now, so maybe I'm just scatterbrained now, but shouldn't that check be if (!constructing)?  RegExp(p, f) sometimes returns p, but new RegExp(p, f) never should, if I read ES5 right (and assume this isn't something noted in errata).  Are our RegExp shell tests all contradicting the spec on this, or something?

But the safety-fix aspect looks good, at least.

Comment 8

[Luke Wagner [:luke]]

(In reply to comment #7)
You're right, see the "D'oh!" in comment 6 :)

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1d477685d70a

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e051419897da