544238 - CodegenLIR doesn't properly clear null-check state at debugger stopping points

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect, P2)

Description

[Edwin Smith]

This can result in a crash (only while debugging) with these steps:

1. access a property on an object, causing implicit null check.
2. the pointer continues to be marked non-null after the property-access instruction, but a debugger can set it to null.
3. access the object again; crash, because the implicit null check
was incorrectly removed.

(this can also happen with copies of the pointer, since FrameState.Value.ins is stale and can result in notNull being incorrectly set on copies of the object reference)

The problem is that CopyPropagation clears its state, however FrameState.value(*).ins is not cleared.  This allows Verifier::emitCheckNull to copy-propagate the notNull bit incorrectly.

Test case:

class C { 
    function g() {
        print("in g")
    }
}
function f(x:C) { 
    x.g()    <-- stop in the debugger, set x = null 
    x.g()    <-- step, code will crash
}
f(new C())

|  0:getlocal1
|    ld3 = ld vars[8]
|    sti vars[16] = ld3
|
|                        stack: C?@ld3
|                        scope: [global]
|                       locals: Object@ldc1 C?@ld3
|  1:callprop {public,t.as$1}::g 0
|    eq1 = eq ld3, 0
|    jt eq1 -> unpatched
|
|    ldc3 = ldc ld3[32]
|    ldc4 = ldc ldc3[48]
|    ialloc1 = ialloc 4
|    sti ialloc1[0] = ld3
|    ld4 = ld ldc4[0]
|    acalli1 = icall [ld4] ( ldc4 0 ialloc1 )
|
|    live ialloc1
|    sti vars[16] = acalli1
|
|                        stack: *@acalli1
|                        scope: [global]
|                       locals: Object@ldc1 C@ld3

BUG: C@ld3 should be C? since the debugger can modify vars[16]

|  4:pop
|                        stack:
|                        scope: [global]
|                       locals: Object@ldc1 C@ld3
|  5:getlocal1
|    ld5 = ld vars[8]
|    sti vars[16] = ld5
|
|                        stack: C@ld5
|                        scope: [global]
|                       locals: Object@ldc1 C@ld5

Note: Local1 is now C@ld5; the JIT correctly issued a new load, however the notNull state is wrong; it should be C?@ld5 

|  6:callprop {public,t.as$1}::g 0
|    ldc5 = ldc ld5[32]

crash will happen since ld5 could be null ad we haven't null-checked it.

|    ldc6 = ldc ldc5[48]
|    ialloc2 = ialloc 4
|    sti ialloc2[0] = ld5
|    ld6 = ld ldc6[0]
|    acalli2 = icall [ld6] ( ldc6 0 ialloc2 )
|
|    live ialloc2
|    sti vars[16] = acalli2
|
|                        stack: *@acalli2
|                        scope: [global]
|                       locals: Object@ldc1 C@ld5
|  9:pop
|                        stack:
|                        scope: [global]
|                       locals: Object@ldc1 C@ld5

Comment 1

[Edwin Smith]

Attachment: Clear Value.ins state at debugger safe points to thwart false notNull propagation

Okay, this patch fixes the problem I set out to fix, namely that the Verifier incorrectly transferred notNull flags from checked variables to copies of the checked varibles, across debugger safe points where the copies could change value.

by itself, this patch makes the Verifier behave the same with CodegenLIR+DEBUGGER and the old CodegenMIR+DEBUGGER, with respect to how it copy-propagates the notNull bit.

it does *not* fix the problem described on this bug, where the notNull bit is incorrectly stale across safe points, for *all* variables.  Need another patch for that.

Comment 2

[Edwin Smith]

The problem in this bug a bit worse than it first appears.  Once code has been verified, all the attributes that the verifier has computed must be considered the static type-check of the function, and even the debugger must not modify any of these static constraints.

The verifier's computation also must not be influenced by the presence of any outside agent (debugger or optimizer).

here's the hard case:  say you're running the ABC interpreter, which has no extra information about the method, other than the bytecode.  If you stop in the method and change the value of a variable.  The debugger probably will let you assign any value.  However, the verifier has computed a static type for the value, which the debugger generally does not know about.  The program's soundness depends on that value not straying outside it's static type.  (say, if it gets copied to the scope chain), or if the type of the variable depended on its known-not-nullability (e.g. String+String => String but null + null => int).

Comment 3

[Rick Reitmaier]

Comment on attachment 425298 [details] [diff] [review]
Clear Value.ins state at debugger safe points to thwart false notNull propagation

i suppose if we could drop out to the interpreter when stepping, we wouldn't have to worry about this case.

Also, it wasn't clear from the context in the patch so just want to confirm that this change affects debugger builds only.

Comment 4

[Edwin Smith]

(In reply to comment #3)
> (From update of attachment 425298 [details] [diff] [review])
> i suppose if we could drop out to the interpreter when stepping, we wouldn't
> have to worry about this case.

deopt is on the way

> Also, it wasn't clear from the context in the patch so just want to confirm
> that this change affects debugger builds only.

right, the intent of the patch was to nudge the jit into bug compatibility (*) with the one that has already shipped, by clearing the copypropagation state at debugger safe points, which indirectly affects notNull bits in FrameState and ultimately the verifier's type model.

(*) the "bug" is that the jit affects the verifier's state at all. see bug 413522.

Comment 5

[Dan Smith]

Not an injection and likely too risky for 10.1.  Targeting for 10.2.

Comment 6

[Andre Kruetzfeldt]

Rick, can you please follow up with this one?

Comment 7

[Dan Smith]

See comment 5, moving to Anza.

Comment 8

[Andre Kruetzfeldt]

Removing Andre blocker, as it has milestone.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.