814396 - IonMonkey: CharCodeAt continues to use input after ool VM call

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Code generated for CharCodeAt may perform an OOL call to linearize a rope input, and continue to use its input registers after the call returns.  Similar to bug 814177, this seems to work with the LSRA allocator but may break on other allocators that do not mark the inputs as live registers after the instruction.

Comment 1

[Nicolas B. Pierron [:nbp]]

I don't see the issue here.  JSString::ensureLinear will cause an exception if the memory cannot be allocated, and the JSString of this function is supposed to be marked such as even if a GC happen, we can still resume the CharCodeAt instruction before using the OsiPoint.  Then the ensureLinear function is mutating the String representation but not the pointer, so using the pointer after the call should be fine whatever your register allocation algorithm.

Marking input register as live during the execution is needed by many functions such as setelem and others mutating operations.  This is a feature of the register allocator to mark register containing potential GC types (String, Object, Value …) as live across the codegen and this *must* remain the default to prevent trivial security issue which are easy to miss.

This is not the same Bug 814177.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #1)
> Marking input register as live during the execution is needed by many
> functions such as setelem and others mutating operations.  This is a feature
> of the register allocator to mark register containing potential GC types
> (String, Object, Value …) as live across the codegen and this *must* remain
> the default to prevent trivial security issue which are easy to miss.

I was going by the comment on liveRegs in LSafepoint:

    // The set of registers which are live after the safepoint. This is empty
    // for instructions marked as calls.

Which seems to pretty clearly indicate that registers which are live only before the call shouldn't be considered live.  It'd be fine to change it I think, and wouldn't really affect anything as the regalloc already uses disjoint registers for defs/temps vs. inputs, so we wouldn't get confused about whether a given register should be treated as live or a def at a safepoint.  Also, this is the only instruction I ran into with this problem while filing bug 812945, so your assertion that tons of stuff depends on this seems strange.  How does setelem depend on this?  There is an OOL call in StoreElementHole, but it doesn't rejoin until the end of the instruction, which is fine (the inputs won't be used anymore).

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> (In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #1)
> > Marking input register as live during the execution is needed by many
> > functions such as setelem and others mutating operations.  This is a feature
> > of the register allocator to mark register containing potential GC types
> > (String, Object, Value …) as live across the codegen and this *must* remain
> > the default to prevent trivial security issue which are easy to miss.
> 
> I was going by the comment on liveRegs in LSafepoint:
> 
>     // The set of registers which are live after the safepoint. This is empty
>     // for instructions marked as calls.

LCharCodeAt is not marked as a call.  Still it sometimes need to make a call and to use liveRegs to spill live registers and to restore them after the call.

liveRegs should be empty before instruction marked as a call because the Register allocator is doing the spilling for us.

Knowing that we need a safepoint only for instruction *making* a call, the previous comment would imply that liveRegs is constantly empty, and thus useless.  Not all instructions *making* a call as marked are as *call* instructions.

So improving this comment would be nice.

Comment 4

[Brian Hackett [Laid off!]]

This is affecting the backtracking allocator (bug 814966) as well.  I think there's some confusion in the earlier comments here.  This is not about instructions marked as calls nor about tracking registers/slots that contain GC things during a call.  Rather, it is just about which registers to include in a safepoint's liveRegs, i.e. those that need to be restored after making an OOL call in the instruction.  The sequence of events here is:

1. LCharCodeAt gets a string which is a rope.
2. An OOL call is made to flatten the string.
3. That OOL call rejoins the inline path, which tries to use its input registers for the string/index as if they are still valid.
4. That string/index may not be live after the instruction, so are not in liveRegs and were not restored by the OOL call.

This seems to me a bug in CharCodeAt for two reasons:

1. It violates the comment on liveRegs by assuming its input registers are valid after the OOL call.
2. This is the only instruction that has been causing this problem for me, and skimming the rest of code generation I do not see any other place where OOL calls rejoin in the middle of the inline path.

This bug is an easy fix, and makes LCharCodeAt behave more like every other instruction.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

(See above comments for context)

Comment 6

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/450321f661e8

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> This is affecting the backtracking allocator (bug 814966) as well.  I think
> there's some confusion in the earlier comments here.  This is not about
> instructions marked as calls nor about tracking registers/slots that contain
> GC things during a call.  Rather, it is just about which registers to
> include in a safepoint's liveRegs, i.e. those that need to be restored after
> making an OOL call in the instruction.  The sequence of events here is:
> 
> 1. LCharCodeAt gets a string which is a rope.
> 2. An OOL call is made to flatten the string.
> 3. That OOL call rejoins the inline path, which tries to use its input
> registers for the string/index as if they are still valid.
> 4. That string/index may not be live after the instruction, so are not in
> liveRegs and were not restored by the OOL call.
> 
> This seems to me a bug in CharCodeAt for two reasons:
> 
> 1. It violates the comment on liveRegs by assuming its input registers are
> valid after the OOL call.

As answered previously, you should fix the comment, not the code in such case!

> 2. This is the only instruction that has been causing this problem for me,
> and skimming the rest of code generation I do not see any other place where
> OOL calls rejoin in the middle of the inline path.

I don't see how this is a CharCodeAt issue, but more an allocator issue.  We want to be able to do such things.  If this is not working this means that you are not filling correctly your safepoints which means this will cause a security critical GC issue.

This patch sounds like a work-around made to hide a general security issue, because the stack can still be inspected during any VM calls, so even inputs of a LIR instruction should be added to the liveRegs vector of the safepoints, such as a GC followed by a dump of the frame cannot lead to a non-handled pointer.


Please fix the original issue coming from the LLVM greedy backtracking register allocator by filling correctly the safepoints and fix the comment as suggested in comment 3.

Mark as this bug as leave open because the current patch does not fix the underlying issue originally reported as a bug caused by charCodeAt codegen.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/450321f661e8

Comment 9

[Brian Hackett [Laid off!]]

OK, I filed bug 821735 for the AllocationIntegrityState (not BacktrackingAllocator) issue.  Can you leave this one alone?

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

While we should strive not to have complex register state in an instruction, I think it's reasonable to expect that ops should be able to make an out-of-line call, then keep using their input registers. It's a bug in the allocator if this isn't possible.