Closed Bug 375651 Opened 17 years ago Closed 17 years ago

"Assertion failure: nbytes != 0" / crash [@ __memcpy] with regexp quantifiers

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: crowderbt)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical?] post 1.8-branch)

Crash Data

Attachments

(2 files, 1 obsolete file)

js> /(.{2,3}){0,2}?t/.exec("abt")
Assertion failure: nbytes != 0, at jsapi.c:1678


Debug assertion:
0   js 	0x000c672c JS_Assert + 70 (jsutil.c:60)
1   js 	0x0001435f JS_malloc + 50 (jsapi.c:1679)
2   js 	0x000c3e11 js_NewStringCopyN + 31 (jsstr.c:2531)
3   js 	0x000ae990 js_ExecuteRegExp + 1481 (jsregexp.c:3490)
4   js 	0x000b010f regexp_exec_sub + 1047 (jsregexp.c:4121)
5   js 	0x000b0283 regexp_exec + 53 (jsregexp.c:4137)
6   js 	0x00057333 js_Invoke + 2954 (jsinterp.c:1353)


Opt crash:

Exception:  EXC_BAD_ACCESS (0x0001)
Codes:      KERN_INVALID_ADDRESS (0x0001) at 0x002ffffe

Thread 0 Crashed:
0   <<00000000>> 	0xffff0f20 __memcpy + 1920 (cpu_capabilities.h:228)
1   js 	0x0006a41d js_NewStringCopyN + 60
2   js 	0x00061cf4 js_ExecuteRegExp + 5993
3   js 	0x00062228 regexp_exec_sub + 758
4   js 	0x0003bb73 js_Invoke + 1843
If you are not the right person to assign this to, please help us find someone that is.
Assignee: general → crowder
I'm getting a different assertion now.

js> /(.{2,3}){0,2}?t/.exec("abt")
Assertion failure: x->cp >= (gData->cpbegin + cap->index), at jsregexp.c:2882
Status: NEW → ASSIGNED
This still crashes trunk opt jsshell in a way that looks exploitable, dereferencing 0x002ffffe. 

In jsshell debug, I'm back to getting the original assertion:
Assertion failure: nbytes != 0, at jsapi.c:1671
Whiteboard: [sg:critical?]
This is one of the bugs hit by the assertion I recently added, iirc.  I'll look at this this afternoon or tomorrow.
try for 1.9?
Flags: blocking1.9?
Crowder, are you going to be able to fix this exploitable crash for Gecko 1.9?
I still think there is something systemically wrong earlier on, here, but this patch prevents the exploitable problem and seems to produce acceptable results.  I'll run the test-suite, then ask mrbkap for review.
Comment on attachment 276027 [details] [diff] [review]
wallpaper patch for this and another issue

Same failure on my little regex suite before and after the patch.  I think it's safe enough.
Attachment #276027 - Flags: review?(mrbkap)
Attachment #276027 - Flags: review?(mrbkap) → review+
See bug 379056 comment 13. Is bug 379056 a dup of this bug? Brian: I think you, mrbkap, and I should try to understand this, maybe next week when you're in town.

/be
Attachment #276027 - Flags: approval1.9?
Depends on: 379056
Comment on attachment 276027 [details] [diff] [review]
wallpaper patch for this and another issue

> #if defined(DEBUG_crowder) || defined(DEBUG_mrbkap)
>                 JS_ASSERT(x->cp >= (gData->cpbegin + cap->index));
>                 JS_ASSERT((int)cap->length <= (gData->cpend - gData->cpbegin));
> #endif

You guys still want this pain?

>+                if (x->cp < gData->cpbegin + cap->index)
>+                    cap->length = 0;
>+                else
>+                    cap->length = x->cp - (gData->cpbegin + cap->index);

I didn't review, but if you want my two cents (you don't have to, since mrbkap r+'d) the way to shrink this in source form (if not compiled; not sure how it optimizes) would be to use a case-local ptrdiff_t delta (say) to capture x->cp - (gData->cpbegin + cap->index), and set cap->length = (delta < 0) ? 0 : (size_t) delta.

/be
Attachment #276027 - Flags: approval1.9? → approval1.9+
I liked Brendan's feedback, so here's another stab.
Attachment #276027 - Attachment is obsolete: true
Attachment #277915 - Flags: review?(mrbkap)
Comment on attachment 277915 [details] [diff] [review]
with brendan's feedback

Do we want to assert that delta > 0? Or are we confident that our alternate approach will fix all cases where the assertion would fire?
I'll add an assertion when I implement the alternative approach.
mrbkap: pinging for review again?
Comment on attachment 277915 [details] [diff] [review]
with brendan's feedback

Sorry -- thought I'd already stamped this.
Attachment #277915 - Flags: review?(mrbkap) → review+
jsregexp.c: 3.162
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical?] → [sg:critical?] post
On the 1.8 branch see also 346090
Group: core-security
Flags: wanted1.8.1.x-
Whiteboard: [sg:critical?] post → [sg:critical?] post 1.8-branch
test checked into 1.9.0, 1.9.1, 1.9.2, tracemonkey. 1.9.3 will get picked up in the next merge.
Crash Signature: [@ __memcpy]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: