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)
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)
1.89 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
2.35 KB,
text/plain
|
Details |
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
Comment 1•17 years ago
|
||
If you are not the right person to assign this to, please help us find someone that is.
Assignee: general → crowder
Reporter | ||
Comment 2•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•17 years ago
|
||
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?]
Assignee | ||
Comment 4•17 years ago
|
||
This is one of the bugs hit by the assertion I recently added, iirc. I'll look at this this afternoon or tomorrow.
Reporter | ||
Comment 6•17 years ago
|
||
Crowder, are you going to be able to fix this exploitable crash for Gecko 1.9?
Assignee | ||
Comment 7•17 years ago
|
||
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.
Assignee | ||
Comment 8•17 years ago
|
||
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)
Updated•17 years ago
|
Attachment #276027 -
Flags: review?(mrbkap) → review+
Comment 9•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Attachment #276027 -
Flags: approval1.9?
Comment 10•17 years ago
|
||
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+
Assignee | ||
Comment 11•17 years ago
|
||
I liked Brendan's feedback, so here's another stab.
Attachment #276027 -
Attachment is obsolete: true
Attachment #277915 -
Flags: review?(mrbkap)
Comment 12•17 years ago
|
||
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?
Assignee | ||
Comment 13•17 years ago
|
||
I'll add an assertion when I implement the alternative approach.
Assignee | ||
Comment 14•17 years ago
|
||
mrbkap: pinging for review again?
Comment 15•17 years ago
|
||
Comment on attachment 277915 [details] [diff] [review] with brendan's feedback Sorry -- thought I'd already stamped this.
Attachment #277915 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 16•17 years ago
|
||
jsregexp.c: 3.162
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 17•17 years ago
|
||
Updated•17 years ago
|
Flags: in-testsuite+
Updated•15 years ago
|
Whiteboard: [sg:critical?] → [sg:critical?] post
Comment 19•15 years ago
|
||
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
Comment 20•14 years ago
|
||
test checked into 1.9.0, 1.9.1, 1.9.2, tracemonkey. 1.9.3 will get picked up in the next merge.
Updated•13 years ago
|
Crash Signature: [@ __memcpy]
You need to log in
before you can comment on or make changes to this bug.
Description
•