Closed
Bug 364350
Opened 18 years ago
Closed 18 years ago
free unitialized memory from js_DestroyRegExp in OOM conditions
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: gavin.reaney, Assigned: gavin.reaney)
Details
(Keywords: fixed1.8.0.10, fixed1.8.1.2)
Attachments
(1 file)
933 bytes,
patch
|
crowderbt
:
review+
jay
:
approval1.8.1.2+
jay
:
approval1.8.0.10+
|
Details | Diff | Splinter Review |
This was noticed when running some OOM testing. The failure point is indicated below: js_DestroyRegExp(JSContext *cx, JSRegExp *re) { if (JS_ATOMIC_DECREMENT(&re->nrefs) == 0) { if (re->classList) { uintN i; for (i = 0; i < re->classCount; i++) { if (re->classList[i].converted) JS_free(cx, re->classList[i].u.bits); //<-- double free re->classList[i].u.bits = NULL; } JS_free(cx, re->classList); } JS_free(cx, re); } } This is caused when js_NewRegExp attempts to destroy a partially constructed regexp (the memory allocated to re->classlist is left uninitialised if EmitREBytecode fails).
Assignee | ||
Comment 1•18 years ago
|
||
Proposed fix - initialise the class list to allow safe destruction.
Attachment #249106 -
Flags: review?(brendan)
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Comment 2•18 years ago
|
||
Brian, can you shepherd this patch in? Gavin: thanks again. If you want to apply for CVS access, I'll vouch for you. /be
Comment 3•18 years ago
|
||
Brendan: Is that your r+?
Updated•18 years ago
|
Attachment #249106 -
Flags: review?(brendan) → review?(crowder)
Comment 4•18 years ago
|
||
Any thoughts or testing on whether this performs better/worse as a memset()?
Comment 5•18 years ago
|
||
(In reply to comment #4) > Any thoughts or testing on whether this performs better/worse as a memset()? Good point -- could be both faster and less code (code size comparison welcome too). /be
Assignee | ||
Comment 6•18 years ago
|
||
(In reply to comment #5) > (In reply to comment #4) > > Any thoughts or testing on whether this performs better/worse as a memset()? > > Good point -- could be both faster and less code (code size comparison welcome > too). > > /be I was in two minds about using memset - I can certainly change it if you think this would be better. My main reason for not using memset was to make the required initialisation more explicit - js_DestroyRegExp checks for 'converted' being JS_FALSE so that is what is set during initialisation. It's not exactly an overwhelming argument though since memset(0) is such a common idiom that its purpose should be clear. It's also more future proof should we later require other members of the structure to be zero'd. In terms of code size, the final binary (for the js shell) is the same size when memset is used instead (build with -0 and no debug). This was with 'gcc version 3.3.5 (Debian 1:3.3.5-13)'. Performance results favour the explicit loop for the more common cases I think. I took some timings using inline assembly to read the processor time stamp counter: extern __inline__ unsigned long long int rdtsc() { unsigned long long int x; __asm__ volatile (".byte 0x0f, 0x31" : "=A" (x)); return x; } memset code: t1 = rdtsc(); memset(re->classList, 0, re->classCount * sizeof(RECharSet)); t2 = rdtsc(); printf("time %d\n", t2 - t1); loop code: t1 = rdtsc(); for (i = 0; i < re->classCount; i++) re->classList[i].converted = JS_FALSE; t2 = rdtsc(); printf("time %d\n", t2 - t1); Results for some values of 'classCount' are given below: classCount loop memset 1 15 97 2 48 100 3 52 103 4 57 106 5 61 109 100 455 415 memset is only faster when classCount is quite large (I didn't quite get around to working out the crossover point but it was more than 20 anyway).
Comment 7•18 years ago
|
||
Hmm, wonder what -Os does. Interesting that memset is slower. 0/null/false are all equal, we count on this in many places and it's the only sane way to go. I'll butt out now ;-). /be
Comment 8•18 years ago
|
||
I was surprised by the memset thing, but it seems that for a large enough chunk of memory, memset() yields a function-call (at least in gcc), even with -Os or -O2, so my results (even looking at assembly code from gcc) tended to look the same. Tomorrow I will investigate the same for Visual Studio.
Assignee | ||
Comment 9•18 years ago
|
||
I've given this a try with a VS2005 express build of the JS shell. The rdtsc function was replaced as follows, but otherwise the test should be identical. static uint64 rdtsc(void) { __asm { rdtsc } } As with GCC, memset is slower for small values of classCount (but the point at which memset becomes faster seems to be less maybe around a classCount of 15). The differences are only a few tens of clock cycles so I wouldn't rule out using memset if you prefer it.
Comment 10•18 years ago
|
||
It seems unlikely that many regex would have significantly more than fifteen character classes in them, so let's optimize for the smaller case. I'll check in the non-memset() version shortly.
Comment 11•18 years ago
|
||
Comment on attachment 249106 [details] [diff] [review] Fix for double free r=me and seeking approval for branches, since this is a good, simple OOM-handling fix
Attachment #249106 -
Flags: review?(crowder)
Attachment #249106 -
Flags: review+
Attachment #249106 -
Flags: approval1.8.1.2?
Attachment #249106 -
Flags: approval1.8.0.10?
Comment 12•18 years ago
|
||
Commited on trunk /cvsroot/mozilla/js/src/jsregexp.c,v <-- jsregexp.c new revision: 3.128; previous revision: 3.127 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 13•18 years ago
|
||
Comment on attachment 249106 [details] [diff] [review] Fix for double free Approved for both branches, a=jay for drivers.
Attachment #249106 -
Flags: approval1.8.1.2?
Attachment #249106 -
Flags: approval1.8.1.2+
Attachment #249106 -
Flags: approval1.8.0.10?
Attachment #249106 -
Flags: approval1.8.0.10+
Comment 14•18 years ago
|
||
Landed on both branches 1.8: Checking in jsregexp.c; /cvsroot/mozilla/js/src/jsregexp.c,v <-- jsregexp.c new revision: 3.99.2.20; previous revision: 3.99.2.19 done 1.8.0: Checking in jsregexp.c; /cvsroot/mozilla/js/src/jsregexp.c,v <-- jsregexp.c new revision: 3.99.2.7.2.6; previous revision: 3.99.2.7.2.5 done
Comment 15•18 years ago
|
||
Thanks a lot, Gavin!
Comment 16•18 years ago
|
||
Changed summary slightly, since this isn't really a double-free but a free() on uninitialized memory
Summary: Double free/crash in js_DestroyRegExp in OOM conditions → free unitialized memory from js_DestroyRegExp in OOM conditions
Assignee | ||
Comment 17•18 years ago
|
||
(In reply to comment #2) > Gavin: thanks again. If you want to apply for CVS access, I'll vouch for you. > > /be Thanks Brendan. I'll look into this early next year.
Assignee | ||
Updated•17 years ago
|
Keywords: fixed1.8.0.10,
fixed1.8.1.2
Updated•17 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•