61788 - xpconnect shouldn't leave garbage in stack from js_AllocStack

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P3)

Description

[John Bandhauer]

In testing brendan's patchs to bug 54743 I ran into problems because xpconnect
is leaving some stack slots uninitialized.

Trivial patch coming.

r= and sr= appreciated!

Comment 1

[John Bandhauer]

Attachment: proposed fix

Comment 2

[Brendan Eich [:brendan]]

Why store JSVAL_NULL (rather than JSVAL_VOID) and worse, why penalize all calls
through this method with those stores followed by the real [fun, this, args...]
stores?  Isn't a simpler fix to store two JSVAL_VOIDs in the else { sp += 2; }
clause of that if(!info->IsGetter() && !info->IsSetter()) peeking at the bottom
of your patch?

/be

Comment 3

[John Bandhauer]

JSVAL_VOID is fine with me. Why should it matter?

I was still seeing crashes if I just fill in those two slots in the 'else' case.
I suspect that one of the calls later (but before all of the stack slots are
full) winds back into native code and out of my request. I have to suspect the
call to JS_GetProperty right below there since adding...

  sp[0] = sp[1] = JSVAL_VOID;

...avoids the crash. But, this is just in this little test case. It seems *well*
worth it to me to be safe here and fill in all the slots considering how much
code can run in the param conversions that follow.

I'll change JSVAL_NULL to JSVAL_VOID. But I think the eager init of the stack
space is a good change. Really, I'd assumed that the engine would be doing this
itself - allocing a stack that would not crash the engine if not fully populated
by the caller before a gc happens.

Comment 4

[Brendan Eich [:brendan]]

JSVAL_NULL works too, bug JSVAL_VOID is a less likely pattern than 0, which can
help when looking at skidmarks.

js_AllocStack is a sharp tool (JS_FRIEND_API) and it doesn't presume to waste
cycles for you.  XPConnect performance may not change much due to doubling the
stores to the stack, but if we can avoid that cost safely, I think we should.

That JS_GetProperty just before *sp++ = fval; *sp++ = OBJECT_TO_JSVAL(obj); does
look like a problem, if it calls out to a getter that suspends the request, or
is an XPConnected native method itself.  Was that the case?  It would be good to
get to the bottom of this.

One alternative to double-stores is to move that JS_GetProperty up above the
js_AllocStack.

/be

Comment 5

[John Bandhauer]

Part of my point was that the stuff that happens in xpcconvert to fill the rest
of the stack slots has plenty of potential to get out of my request. Maybe you
think I'm just being sloppy, but I think that this is a necessary (and very
minimal) evil.

Comment 6

[Brendan Eich [:brendan]]

I don't think it's sloppy, never said that.  js_AllocStack is used in the JS
engine itself, and every use has straight-line code that stores repeatedly via
*sp++ = ....  I was hoping XPConnect could do the same, but if the control flow
is deep and can go out of the request, then your patch is right and necessary.

/be

Comment 7

[John Bandhauer]

Attachment: another proposed fix

Comment 8

[Brendan Eich [:brendan]]

sr=[e-mail] on that patch, thanks.

/be

Comment 9

[John Bandhauer]

[beat me to it!]

How about this patch then? I leave in the init'ing of the slots for safety. But
I only allocate as many slots as I really need (maybe none at all!). And I only
double init the slots of the args-to-be-converted, and not the funtion and
'this' which are set early. As far as I can tell js_AllocSlot is not going to do
a gc, so I'm OK in putting the function property I get in the local fval while
it is called.

r= mccabe?

Comment 10

[John Bandhauer]

Attachment: a slightly better fix

Comment 11

[John Bandhauer]

I like this one a little better. It avoids the extras !'s in the test to figure
out if we're calling a function or not. And I added comments to explain the
logic and to XXX a future error reporting enhancement.

Sorry to be throwing more of the same at you.

Thanks!

Comment 12

[Brendan Eich [:brendan]]

I'm still sr='ing, but just noticed the uint8 local variables -- those can cost
when comparing, assigning to, and assigning from natural-sized unsigneds, which
JS typedefs as uintN (N >= 32 in the modern era, N >= 16 back when Win16 was
supported).  It may not matter in this case, and I haven't analyzed the uses of
these variables, but maybe a quick experiment and code size measurements can decide?

/be

Comment 13

[Mike McCabe]

r=mccabe, ditto uint8 comments, unless you need uint8 overflow behavior for some
reason.

what's argc for getters/setters?

Comment 14

[Brendan Eich [:brendan]]

getter: argc == 0; setter: argc == 1.

/be

Comment 15

[John Bandhauer]

fix checked in

Comment 16

[Phil Schwartau]

Marking Verified -