Closed Bug 503452 Opened 15 years ago Closed 13 years ago

Reference-counting failure in OOM case when cloning a regular expression

Categories

(Core :: JavaScript Engine, defect)

1.9.1 Branch
defect
Not set
critical

Tracking

()

RESOLVED INVALID

People

(Reporter: Waldo, Assigned: Waldo)

References

()

Details

(Whiteboard: [sg:nse] 1.9.1 branch and earlier)

Attachments

(1 file, 1 obsolete file)

See URL.  If js_SetLastIndex fails, we don't increment re->nrefs but do set the private slot.  The latest patch in bug 493457 fixes this en passant, but I want to note it here so we can fix it in stable branches and such -- although, as far as I can tell, it's impossible to hit, because js_SetLastIndex cannot fail with an INT_FITS_IN_JSVAL() number.  (Or so I see if I trace through JS_SetReservedSlot, but it's possible I could have misread this -- hence the security-sensitive flag.)

I'll spare you the shell transcript with a hacked shell, with all JSOPTION_COMPILE_N_GO uses removed from that shell, with evalcx fun, with instruction single-stepping, with eax-munging, and finally with a song and dance routine that demonstrates the theoretical presence of a problem if it weren't for the index-set always being to 0 (of course noted *after* the song and dance routine).
Whiteboard: [sg:nse]
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #393339 - Flags: review?(igor)
Inside the js/src firewall we can, and do, assume JS_SetPrivate is infallible.

/be
For a branch backport patch, I figure it's better as a general policy to be paranoid, even if this particular case it's not needed.
JS_SetPrivate is infallible on all maintained branches.

/be
I didn't dispute that, did I?
(In reply to comment #5)
> I didn't dispute that, did I?

Then what is your justification for useless code?

Lose the attitude, "paranoia" does not justify illogical "general policy" which is neither general nor policy.

/be
General rule for myself, then, not interested in disputing the precise term.  If I do a backport -- and if I rely on a verifiable property of a method's operation -- then I add a debug-only check that does not itself add useless code except in debug builds, in which the marginal cost of the check is nearly nil (could even be nil if no one has to debug through it, as I expect no one to be able to notice a few CPU cycles).

I really don't see why there's such animosity to a debug-only check (suggested by an API flaw) in non-trunk code, when trunk's APIs have been fixed to avoid the problem entirely.  Don't we have better things to argue about than debug-only checks in old branches?  I really don't see why we're even arguing about this.
We have such checks, e.g.

#ifdef DEBUG
            JSPropCacheEntry *entry =
#endif
            js_FillPropertyCache(cx, scopeChain,
                                 scopeIndex, protoIndex, pobj,
                                 (JSScopeProperty *) prop, false);
            JS_ASSERT(entry);

to future-proof against a day when, in this example, js_FillPropertyCache becomes fallible. But no such future should be proofed (sic) for JS_SetPrivate:

$ grep JS_SetPrivate *.cpp
jsapi.cpp:JS_SetPrivate(JSContext *cx, JSObject *obj, void *data)
jsdbgapi.cpp:    JS_SetPrivate(cx, obj, p);
jsfile.cpp:    if (!JS_SetPrivate(cx, obj, file)) {
jstracer.cpp:                    JS_SetPrivate(cx, JSVAL_TO_OBJECT(fp->argsobj), fp);

jsfile.cpp is a wasteland, elsewhere we've removed internal uses except for two (not sure either is well-motivated right now) in favor of

jsbool.cpp:        obj->fslots[JSSLOT_PRIVATE] = bval;

etc. This was intentional, with cvs annotate showing the bug numbers; let's not go back on it.

Every bit of source complexity (we have lots) hurts, including on maintenance branches. Paranoia is called for when calling out of module to something with a poor track record, or an API that screams "fallible". No argument there. But JS_SetPrivate is in-module.

If the issue were future-proofing against potential fallibility down the road in the trunk, where we had no reason to insist on perpetual infallibitility (private fslot, never again to be a dslot requiring object locking as in the bad old days!), then *maybe*. But the current maintenance branches wouldn't reach any such indefinitely-far-off future.

Not only won't JS_SetPrivate devolve to return false, we shouldn't even be calling it internally if we don't have good reason (which could exist, e.g., converting JSVAL_VOID to a null return -- worth looking into re: jsdbgapi.cpp and jstracer.cpp uses).

/be
Comment on attachment 393339 [details] [diff] [review]
Patch for 191, also applies cleanly with fuzz (and name-change) to 190

>-    if (!JS_SetPrivate(cx, clone, re) || !js_SetLastIndex(cx, clone, 0)) {
>+#ifdef DEBUG
>+    JSBool ok =
>+#endif
>+        JS_SetPrivate(cx, clone, re);
>+    JS_ASSERT(ok);

Nit: JS_SetPrivate cannot fail. Asserting that just adds an extra noise.
Attachment #393339 - Flags: review?(igor) → review+
(In reply to comment #8)
> Every bit of source complexity (we have lots) hurts, including on maintenance
> branches.

Disagree with the latter entirely, and I would wager a Sundance steak that we will not otherwise modify js_CloneRegExp on any current maintenance branches, in which case this truly would have no impact.
Hygienic change for branches, conceivably important if I misread what js_SetLastIndex does and if an attacker can get lucky with triggering an OOM failure or similar...
Attachment #393339 - Attachment is obsolete: true
Attachment #393668 - Flags: approval1.9.1.4?
Attachment #393668 - Flags: approval1.9.0.15?
(In reply to comment #10)
> (In reply to comment #8)
> > Every bit of source complexity (we have lots) hurts, including on maintenance
> > branches.
> 
> Disagree with the latter entirely, and I would wager a Sundance steak that we
> will not otherwise modify js_CloneRegExp on any current maintenance branches,
> in which case this truly would have no impact.

Why are you wasting time barking up the wrong tree? By this same argument there is zero reason to bloat the branch patch with noise, which Igor also noted.

/be
(In reply to comment #11)
> Created an attachment (id=393668) [details]
> With the assert removed
> 
> Hygienic change for branches, conceivably important if I misread what
> js_SetLastIndex does and if an attacker can get lucky with triggering an OOM
> failure or similar...

js_SetLastIndex(cx, clone, 0) is infallible on 1.9.0 and even 1.8.1 as the slot is allocated among the fixed free slots.
Whiteboard: [sg:nse] → [sg:nse] 1.9.1 branch and earlier
Version: Trunk → 1.9.1 Branch
Based on the above discussion, it's not clear to me if we actually need to take this patch on the 1.9.1 and 1.9.0 branches. Waldo / Igor / Brendan?
Comment on attachment 393668 [details] [diff] [review]
With the assert removed

Clearing approval requests until we get an answer to comment 14.
Attachment #393668 - Flags: approval1.9.1.4?
Attachment #393668 - Flags: approval1.9.0.15?
Comment on attachment 393668 [details] [diff] [review]
With the assert removed

I think we should.  It is not at all obvious from simply reading that js_SetLastIndex(cx, clone, 0) is infallible.  Demonstrating that requires knowledge of the infallibility of several different method calls which compose to form its effect, all of which are usually fallible but for the specific value being 0, the specifics of the RegExp class, and a hefty amount of code implementing the processing of both.  Making that many complex reliances and conditioning memory correctness on doing so is a sure loser in my book.
Attachment #393668 - Flags: approval1.9.1.4?
Attachment #393668 - Flags: approval1.9.0.15?
Sam: Igor and I say there's no need to take that patch.

Jeff: the tracemonkey version of jsregexp.cpp avoids the issue. Why don't we backport that and minimize inter-tree differences? Otherwise you are making work for no real reason.

/be
Backporting the TM version is significantly less trivial than the minimal changes proposed here -- the most minimal possible to avoid readability error.  I'm wary of doing that extra work for no reason; I really don't think anyone is going to touch this code on branches outside of audits.
So this is not a security-sensitive bug, just a code inefficiency or confusion issue. Why does it need to be restricted? Why does it need to be fixed on any branch?

/be
Comment on attachment 393668 [details] [diff] [review]
With the assert removed

Since he's module owner, I'm deferring to Brendan here. If an agreement is reached later in this bug and we do want to take a patch, please state that when requesting approval.
Attachment #393668 - Flags: approval1.9.1.4?
Attachment #393668 - Flags: approval1.9.0.15?
Because absent any context but knowing about hold/drop, there would be a clear bug with the consequence being a double-free, and I'd rather not have anyone else waste time on that rathole when modifying a few lines in a nearly-mechanical eliminates the confusion.

Sigh.
We have non-local knowledge not expressed by signatures or other types, we use it all the time. *All* the time.

Static type systems that can express such non-local/high-level invariants are a research problem. In the mean time, we avoid code bloat (which creates new bug habitat) and runtime inefficiency using our personal wetware.

I don't know why this case of using non-local/non-type-checked knowledge is one to waste another minute on. It is not a real bug. It is a potential bug only if we lost our minds and removed fixed slots, going back to 2006 -- which we will not do.

Please, let's focus on real bugs, and potential bugs where the risk of the bug becoming real is non-zero.

/be
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: