460886 - TM: "Assertion failure: end >= begin" with .substring()

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js -j 
js> for (var j = 0; j < 5; ++j) { "".substring(5); }

Assertion failure: end >= begin, at jsstr.cpp:796

js -j 
for (var j = 0; j < 5; ++j) { "".substring(-60000); } 

Crash [@ js_NewStringCopy] with the invalid memory address being some function of the number in the testcase.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Ewww. That swap at the end of SubstringTail is really ugly.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 344003 [details] [diff] [review]
Fix

> #ifdef JS_TRACER
> static JSString* FASTCALL
> String_p_substring(JSContext* cx, JSString* str, int32 begin, int32 end)
> {
>-    JS_ASSERT(end >= begin);
>     JS_ASSERT(JS_ON_TRACE(cx));
>-    return js_NewDependentString(cx, str, (size_t)begin, (size_t)(end - begin));
>+
>+    int32 length = JSSTRING_LENGTH(str);

Not int32 -- size_t (consolidate the widening to jsdouble within SubstringTail).

>+    return SubstringTail(cx, str, length, begin, end);
> }
> 
> static JSString* FASTCALL
> String_p_substring_1(JSContext* cx, JSString* str, int32 begin)
> {
>-    int32 end = JSSTRING_LENGTH(str);
>-    JS_ASSERT(end >= begin);
>     JS_ASSERT(JS_ON_TRACE(cx));
>-    return js_NewDependentString(cx, str, (size_t)begin, (size_t)(end - begin));
>+
>+    int32 length = JSSTRING_LENGTH(str);

Ditto.

> function testif() {
>-	var q = 0;
>-	for (var i = 0; i < 100; i++) {
>-		if ((i & 1) == 0)
>-			q++;
>-		else
>-			q--;
>-	}
>+        var q = 0;
>+        for (var i = 0; i < 100; i++) {
>+                if ((i & 1) == 0)
>+                        q++;
>+                else
>+                        q--;
>+        }

Overindented! What happened?

You sure you want all that trace-tests.js blame?

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #2)
> Overindented! What happened?

Half of trace-tests.js expects tabstop=4 and half of it expects tabstop=8. I'll revert those changes because it's not worth going back through now.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/35c34996d80e

Comment 6

[Bob Clary [:bc] (inactive)]

the trace-test.js portion has been added to js1_8_1/trace/trace-test.js

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-460886-01.js

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-460886-02.js

I could not reproduce with either testcase.

Comment 9

[Bob Clary [:bc] (inactive)]

verified no failures mozilla-central, tracemonkey modulo comment 8

Comment 10

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 11

[Bob Clary [:bc] (inactive)]

test checked into 1.9.0, 1.9.1, 1.9.2, tracemonkey. 1.9.3 will get picked up in the next merge.