Last Comment Bug 364350 - free unitialized memory from js_DestroyRegExp in OOM conditions
: free unitialized memory from js_DestroyRegExp in OOM conditions
Status: RESOLVED FIXED
: fixed1.8.0.10, fixed1.8.1.2
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- major (vote)
: ---
Assigned To: Gavin Reaney
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-12-19 07:50 PST by Gavin Reaney
Modified: 2007-01-17 04:47 PST (History)
5 users (show)
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix for double free (933 bytes, patch)
2006-12-19 07:53 PST, Gavin Reaney
crowderbt: review+
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Review

Description Gavin Reaney 2006-12-19 07:50:53 PST
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).
Comment 1 Gavin Reaney 2006-12-19 07:53:41 PST
Created attachment 249106 [details] [diff] [review]
Fix for double free

Proposed fix - initialise the class list to allow safe destruction.
Comment 2 Brendan Eich [:brendan] 2006-12-19 08:52:21 PST
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 Brian Crowder 2006-12-19 09:31:49 PST
Brendan:  Is that your r+?
Comment 4 Brian Crowder 2006-12-19 11:33:59 PST
Any thoughts or testing on whether this performs better/worse as a memset()?
Comment 5 Brendan Eich [:brendan] 2006-12-19 11:53:47 PST
(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
Comment 6 Gavin Reaney 2006-12-19 15:45:35 PST
(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 Brendan Eich [:brendan] 2006-12-19 16:39:44 PST
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 Brian Crowder 2006-12-19 20:26:45 PST
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.
Comment 9 Gavin Reaney 2006-12-20 05:32:10 PST
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 Brian Crowder 2006-12-20 09:27:17 PST
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 Brian Crowder 2006-12-20 09:31:35 PST
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
Comment 12 Brian Crowder 2006-12-20 15:02:08 PST
Commited on trunk

/cvsroot/mozilla/js/src/jsregexp.c,v  <--  jsregexp.c
new revision: 3.128; previous revision: 3.127
done
Comment 13 Jay Patel [:jay] 2006-12-20 15:26:02 PST
Comment on attachment 249106 [details] [diff] [review]
Fix for double free

Approved for both branches, a=jay for drivers.
Comment 14 Brian Crowder 2006-12-20 15:49:19 PST
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 Brian Crowder 2006-12-20 15:49:44 PST
Thanks a lot, Gavin!
Comment 16 Brian Crowder 2006-12-20 15:51:55 PST
Changed summary slightly, since this isn't really a double-free but a free() on uninitialized memory
Comment 17 Gavin Reaney 2006-12-22 16:03:39 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.