Regression range points to a67b7aa2cb773264324581eb302653698c29e19f.

Looks like a bad interpretation of the results of the liveness analysis. Here's an annotated repro:

try {
  with (x) {
    // Branch to create a merge
    if (x) {}
    // Potentially throwing statement where r4 is dead (and not written to).
    bar();
  }
} catch (e) {
    // r4 is not used, so it needn't be live, but...
}

// ... r4 is live here, because ...

try {
  with (x) {
    try {
      // ... r4 is never written ...
    } finally {
      // ... and r4 is the "result" register for rethrows (in a dead branch,
      // but we don't know that).
    }
  }
} catch (e) {}

Before the bar() call, r4 is dead (as bar() unconditionally writes it as part of its LoadLookupSlotForCall runtime call). There's a merge because of the "if", so r4 is set to "optimized-out" as part of the merge.

Then, r4 is reused (since it's a temporary) as the "result" register for the finally block, in case there's an exception to rethrow (which there isn't). But, since r4 is never written, its liveness escapes the try block, and leaks all the way up to the first catch block.

So r4 is live in the catch block, but set to optimized-out before the bar() call. So, the IfException merge from bar() to the catch tries to merge optimized-out, and so fails this DCHECK.

My suggestions for a fix are either

a) Don't set dead environment values to optimized out, but rather e.g. undefined, or pick one of the inputs arbitrarily (and remove this DCHECK), or
b) Somehow keep track of temporary register stack depth in the bytecode and take that into account in the liveness analysis.

Option a) seems simpler, though I worry that it might hide more complex issues if those dead environment values are ever accidentally accessed.

From offline discussion, jarin@ is not a fan of a) because optimized-out has helped us find bugs in the past.

I can't see an easy way of doing b), without heavily bloating the bytecode with metadata on temporary registers. However, for this case we could LdaUndefined the result register to ensure that it always has a value (i.e. is always killed). This would be a bit like solution b), though there may be other similar cases that we haven't caught yet.

If we go down this path, we could enforce a requirement that no temporaries are live on function entry -- it won't catch all similar cases to this one, but it should catch a few.

+rmcilroy for discussion.

Re: 4 - we should be able to say no registers are live on function entry except for the "incoming_new_target_or_generator_register" shouldn't we, the bytecode should never assume any are. 

Regarding r4, so the issue is that we assign it for the result register, but since the try/finally is empty there is never a result written to it.  Could we instead change the try_finally_builder to track whether the result register get's used, and only load it in the finally block if it's used?

I think that'll be equivalent to generating the LdaUndefined always, since we always have an exception handler for the try block.

I'm not sure I understand, where were you proposing adding the LdaUndefined? At the point we allocate the result register? I guess that should also work, although it's one extra bytecode compared to my proposal. Could you add a comment with the bytecode and the expected change to the bytecode?

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

It would be at the end of the try block, once we know that the handler isn't taken.

function foo() {
    try {}
    finally {}
}


   12 E> 0x3e819202c8b2 @    0 : 91                StackCheck 
         0x3e819202c8b3 @    1 : 1f ff f9          Mov <context>, r2
         0x3e819202c8b6 @    4 : 03 ff             LdaSmi [-1]
         0x3e819202c8b8 @    6 : 1e fb             Star r0   <- r0 (the token) is written.
                                                             <- we should LdaUndefined r1 somewhere around here.
         0x3e819202c8ba @    8 : 78 07             Jump [7] (0x3e819202c8c1 @ 15)
         0x3e819202c8bc @   10 : 1e fa             Star r1   <- r1 (result) is written in the (implicit) handler.
         0x3e819202c8be @   12 : 02                LdaZero 
         0x3e819202c8bf @   13 : 1e fb             Star r0   <- r0 (the token) is written.
         0x3e819202c8c1 @   15 : 06                LdaTheHole 
         0x3e819202c8c2 @   16 : 92                SetPendingMessage 
         0x3e819202c8c3 @   17 : 1e f9             Star r2
         0x3e819202c8c5 @   19 : 92                SetPendingMessage 
         0x3e819202c8c6 @   20 : 02                LdaZero 
         0x3e819202c8c7 @   21 : 5f fb             TestEqualStrictNoFeedback r0  <- r0 (the token) is tested.
         0x3e819202c8c9 @   23 : 86 05             JumpIfFalse [5] (0x3e819202c8ce @ 28)
         0x3e819202c8cb @   25 : 1d fa             Ldar r1   <- r1 (result) is conditionally read.
         0x3e819202c8cd @   27 : 94                ReThrow 
         0x3e819202c8ce @   28 : 04                LdaUndefined 
   43 S> 0x3e819202c8cf @   29 : 95                Return 
Constant pool (size = 0)
Handler Table (size = 48)
   from   to       hdlr
  (   4,   4)  ->    10 (prediction=0, data=2)

This DCHECK is to help find bugs, and doesn't affect correctness of the generated code. Would we say that it's reasonable to decrease the severity of this? +hablich to help answer this.

If it doesn't affect the correctness of the generated code I think it's fine to reduce severity and drop the security impact labels.

re: #11: Right, so by "once we know that the handler isn't taken" you mean "once we are generating the code for the fallthrough"? That sounds fine to me. Do we have a similar issue with breaks/continues which also don't write to the result register (since "CommandUsesAccumulator" returns false)?

One other thought - could we just do some implicit union of the register liveness at the top of the try block with the liveness at the top of the exception handler (the finally block) - i.e. treat it as an implicit conditional jump from the top of the try block to the handler, since there could be such a jump if an exception is thrown. Would this be another way of solving the issue? 

CommandUsesAccumulator is used for the *read* of the result register, not the *write*. That said, perhaps we do have this issue for break/continue, and perhaps we should *write* the result register whenever CommandUsesAccumulator is *false* to avoid it.

Regarding unioning liveness at the top of the try block with the liveness at the top of the finally block -- that's necessarily there already for correctness (more precisely, there is a union between the liveness of any bytecode that can throw, and the top of the exception handler, which itself 'inherits' liveness from the finally block because of normal control flow). In fact, that is what is causing the issue -- r4 is live in the finally, and thus made live in the top of the try.

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e5df5bd04462f210a548c9d8336e5449c4f09173

commit e5df5bd04462f210a548c9d8336e5449c4f09173
Author: Leszek Swirski <leszeks@chromium.org>
Date: Fri Aug 25 16:31:24 2017

[ignition] Always write the deferred command result register

For deferred commands (such as in try-finally), some deferred commands
save and restore the accumulator using a result register (e.g. return,
throw, rethrow), while others don't (e.g. break, continue,
fall-through).

However, conditionally reading this result register that may not ever be
written caused it to be considered live from the start of the function,
as far as the liveness analysis could statically tell.

Now, we write the result register for all deferred commands, including
the fall-through. As a micro-optimization, we re-use the Smi command
tokeen to clobber the result, rather than emitting an LdaUndefined.

Bug:  chromium:758472 
Change-Id: I2ea65e2249b40ee6403216e654a8bb88d50bec3b
Reviewed-on: https://chromium-review.googlesource.com/635592
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47612}
[modify] https://crrev.com/e5df5bd04462f210a548c9d8336e5449c4f09173/src/interpreter/bytecode-generator.cc
[modify] https://crrev.com/e5df5bd04462f210a548c9d8336e5449c4f09173/test/cctest/interpreter/bytecode_expectations/AsyncGenerators.golden
[modify] https://crrev.com/e5df5bd04462f210a548c9d8336e5449c4f09173/test/cctest/interpreter/bytecode_expectations/ForAwaitOf.golden
[modify] https://crrev.com/e5df5bd04462f210a548c9d8336e5449c4f09173/test/cctest/interpreter/bytecode_expectations/ForOf.golden
[modify] https://crrev.com/e5df5bd04462f210a548c9d8336e5449c4f09173/test/cctest/interpreter/bytecode_expectations/ForOfLoop.golden
[modify] https://crrev.com/e5df5bd04462f210a548c9d8336e5449c4f09173/test/cctest/interpreter/bytecode_expectations/Generators.golden
[modify] https://crrev.com/e5df5bd04462f210a548c9d8336e5449c4f09173/test/cctest/interpreter/bytecode_expectations/StandardForLoop.golden
[modify] https://crrev.com/e5df5bd04462f210a548c9d8336e5449c4f09173/test/cctest/interpreter/bytecode_expectations/TryFinally.golden

ClusterFuzz has detected this issue as fixed in range 47611:47612.

Detailed report: https://clusterfuzz.com/testcase?key=4863599620390912

Fuzzer: inferno_js_fuzzer
Job Type: linux_asan_d8_dbg
Platform Id: linux

Crash Type: DCHECK failure
Crash Address: 
Crash State:
  other->values_[index] != builder()->jsgraph()->OptimizedOutConstant() in bytecod
  v8::internal::compiler::BytecodeGraphBuilder::Environment::Merge
  v8::internal::compiler::BytecodeGraphBuilder::MergeIntoSuccessorEnvironment
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=47388:47389
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_d8_dbg&range=47611:47612

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=4863599620390912

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

ClusterFuzz testcase 4863599620390912 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot