Change debug-evaluate from a whitelist to a blacklist approach

This CL changes how variables are resolved during debug evaluate.
We now re-parse the whole script when creating a ScopeIterator.
This gives us accurate scope information for all parent scopes of the
closure in which we stopped. Using this information, we build
blacklists of stack-allocated variables. Each context on the chain
in between the closure context up to the original native context is
wrapped in a debug-evaluate context with such a blacklist attached.
Variable lookup for debug-evalute contexts then works as follows:

  1) Look up in the materialized stack variables (stayed the same).
  2) Check the blacklist to find out whether to abort further lookup.
  3) Look up in the original context.

Steps 1-3 is repeated for each debug-evaluate context, since they
mirror the original context chain.

R=ulan@chromium.org, yangguo@chromium.org

Change-Id: Ied8e5786772c70566da9627ee3b7eff066fba2b4
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1795354Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Commit-Queue: Simon Zünd <szuend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63666}

src/debug/debug-evaluate.cc

@@ -174,31 +174,31 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
   //  - To make stack-allocated variables visible, we materialize them and
   //    use a debug-evaluate context to wrap both the materialized object and
   //    the original context.
-  //  - We use the original context chain from the function context to the
-  //    native context.
+  //  - We also wrap all contexts on the chain between the original context
+  //    and the function context.
   //  - Between the function scope and the native context, we only resolve
-  //    variable names that the current function already uses. Only for these
-  //    names we can be sure that they will be correctly resolved. For the
-  //    rest, we only resolve to with, script, and native contexts. We use a
-  //    whitelist to implement that.
+  //    variable names that are guaranteed to not be shadowed by stack-allocated
+  //    variables. Contexts between the function context and the original
+  //    context have a blacklist attached to implement that.
   // Context::Lookup has special handling for debug-evaluate contexts:
   //  - Look up in the materialized stack variables.
+  //  - Check the blacklist to find out whether to abort further lookup.
   //  - Look up in the original context.
-  //  - Check the whitelist to find out whether to skip contexts during lookup.
-  for (; scope_iterator_.InInnerScope(); scope_iterator_.Next()) {
+  for (; !scope_iterator_.Done(); scope_iterator_.Next()) {
     ScopeIterator::ScopeType scope_type = scope_iterator_.Type();
     if (scope_type == ScopeIterator::ScopeTypeScript) break;
     ContextChainElement context_chain_element;
-    if (scope_type == ScopeIterator::ScopeTypeLocal ||
-        scope_iterator_.DeclaresLocals(ScopeIterator::Mode::STACK)) {
+    if (scope_iterator_.InInnerScope() &&
+        (scope_type == ScopeIterator::ScopeTypeLocal ||
+         scope_iterator_.DeclaresLocals(ScopeIterator::Mode::STACK))) {
       context_chain_element.materialized_object =
           scope_iterator_.ScopeObject(ScopeIterator::Mode::STACK);
     }
     if (scope_iterator_.HasContext()) {
       context_chain_element.wrapped_context = scope_iterator_.CurrentContext();
     }
-    if (scope_type == ScopeIterator::ScopeTypeLocal) {
-      context_chain_element.whitelist = scope_iterator_.GetNonLocals();
+    if (!scope_iterator_.InInnerScope()) {
+      context_chain_element.blacklist = scope_iterator_.GetLocals();
     }
     context_chain_.push_back(context_chain_element);
   }
@@ -214,7 +214,7 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
     scope_info->SetIsDebugEvaluateScope();
     evaluation_context_ = factory->NewDebugEvaluateContext(
         evaluation_context_, scope_info, element.materialized_object,
-        element.wrapped_context, element.whitelist);
+        element.wrapped_context, element.blacklist);
   }
 }
 

src/debug/debug-evaluate.h

@@ -83,7 +83,7 @@ class DebugEvaluate : public AllStatic {
     struct ContextChainElement {
       Handle<Context> wrapped_context;
       Handle<JSObject> materialized_object;
-      Handle<StringSet> whitelist;
+      Handle<StringSet> blacklist;
     };
 
     Handle<Context> evaluation_context_;

src/debug/debug-scopes.cc

@@ -267,18 +267,6 @@ void ScopeIterator::TryParseAndRetrieveScopes(ScopeIterator::Option option) {
                          ? scope_chain_retriever.ClosureScope()
                          : literal_scope;
 
-    if (option == COLLECT_NON_LOCALS) {
-      DCHECK(non_locals_.is_null());
-
-      non_locals_ = closure_scope_->CollectNonLocals(isolate_, info_,
-                                                     StringSet::New(isolate_));
-      if (!closure_scope_->has_this_declaration() &&
-          closure_scope_->HasThisReference()) {
-        non_locals_ = StringSet::Add(isolate_, non_locals_,
-                                     isolate_->factory()->this_string());
-      }
-    }
-
     CHECK(DeclarationScope::Analyze(info_));
     if (ignore_nested_scopes) {
       current_scope_ = closure_scope_;
@@ -391,6 +379,23 @@ void ScopeIterator::AdvanceToNonHiddenScope() {
   } while (current_scope_->is_hidden());
 }
 
+void ScopeIterator::AdvanceContext() {
+  DCHECK(!context_->IsNativeContext());
+  context_ = handle(context_->previous(), isolate_);
+
+  // While advancing one context, we need to advance at least one
+  // scope, but until we hit the next scope that actually requires
+  // a context. All the locals collected along the way build the
+  // blacklist for debug-evaluate for this context.
+  locals_ = StringSet::New(isolate_);
+  do {
+    if (!current_scope_ || !current_scope_->outer_scope()) break;
+
+    current_scope_ = current_scope_->outer_scope();
+    CollectLocalsFromCurrentScope();
+  } while (!current_scope_->NeedsContext());
+}
+
 void ScopeIterator::Next() {
   DCHECK(!Done());
 
@@ -414,11 +419,17 @@ void ScopeIterator::Next() {
       context_ = handle(context_->previous(), isolate_);
     }
   } else if (!inner) {
-    DCHECK(!context_->IsNativeContext());
-    context_ = handle(context_->previous(), isolate_);
+    AdvanceContext();
   } else {
     DCHECK_NOT_NULL(current_scope_);
     AdvanceToNonHiddenScope();
+
+    if (!InInnerScope() && current_scope_ != closure_scope_) {
+      // Edge case when we just go past {closure_scope_}. This case
+      // already needs to start collecting locals for the blacklist.
+      locals_ = StringSet::New(isolate_);
+      CollectLocalsFromCurrentScope();
+    }
   }
 
   UnwrapEvaluationContext();
@@ -576,13 +587,21 @@ bool ScopeIterator::SetVariableValue(Handle<String> name,
   return false;
 }
 
-Handle<StringSet> ScopeIterator::GetNonLocals() { return non_locals_; }
-
 bool ScopeIterator::ClosureScopeHasThisReference() const {
   return !closure_scope_->has_this_declaration() &&
          closure_scope_->HasThisReference();
 }
 
+void ScopeIterator::CollectLocalsFromCurrentScope() {
+  DCHECK(locals_->IsStringSet());
+  for (Variable* var : *current_scope_->locals()) {
+    if (var->location() == VariableLocation::PARAMETER ||
+        var->location() == VariableLocation::LOCAL) {
+      locals_ = StringSet::Add(isolate_, locals_, var->name());
+    }
+  }
+}
+
 #ifdef DEBUG
 // Debug print of the content of the current scope.
 void ScopeIterator::DebugPrint() {

src/debug/debug-scopes.h

@@ -80,7 +80,7 @@ class ScopeIterator {
   bool ClosureScopeHasThisReference() const;
 
   // Populate the set with collected non-local variable names.
-  Handle<StringSet> GetNonLocals();
+  Handle<StringSet> GetLocals() { return locals_; }
 
   // Similar to JSFunction::GetName return the function's name or it's inferred
   // name.
@@ -112,7 +112,7 @@ class ScopeIterator {
   Handle<JSFunction> function_;
   Handle<Context> context_;
   Handle<Script> script_;
-  Handle<StringSet> non_locals_;
+  Handle<StringSet> locals_;
   DeclarationScope* closure_scope_ = nullptr;
   Scope* start_scope_ = nullptr;
   Scope* current_scope_ = nullptr;
@@ -124,6 +124,8 @@ class ScopeIterator {
 
   void AdvanceOneScope();
   void AdvanceToNonHiddenScope();
+  void AdvanceContext();
+  void CollectLocalsFromCurrentScope();
 
   int GetSourcePosition();
 

src/heap/factory.cc

@@ -1549,8 +1549,8 @@ Handle<Context> Factory::NewDebugEvaluateContext(Handle<Context> previous,
                                                  Handle<ScopeInfo> scope_info,
                                                  Handle<JSReceiver> extension,
                                                  Handle<Context> wrapped,
-                                                 Handle<StringSet> whitelist) {
-  STATIC_ASSERT(Context::WHITE_LIST_INDEX == Context::MIN_CONTEXT_SLOTS + 1);
+                                                 Handle<StringSet> blacklist) {
+  STATIC_ASSERT(Context::BLACK_LIST_INDEX == Context::MIN_CONTEXT_SLOTS + 1);
   DCHECK(scope_info->IsDebugEvaluateScope());
   Handle<HeapObject> ext = extension.is_null()
                                ? Handle<HeapObject>::cast(the_hole_value())
@@ -1565,7 +1565,7 @@ Handle<Context> Factory::NewDebugEvaluateContext(Handle<Context> previous,
   c->set_native_context(previous->native_context());
   c->set_extension(*ext);
   if (!wrapped.is_null()) c->set(Context::WRAPPED_CONTEXT_INDEX, *wrapped);
-  if (!whitelist.is_null()) c->set(Context::WHITE_LIST_INDEX, *whitelist);
+  if (!blacklist.is_null()) c->set(Context::BLACK_LIST_INDEX, *blacklist);
   return c;
 }
 

src/objects/contexts.cc

@@ -175,7 +175,6 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
   Isolate* isolate = context->GetIsolate();
 
   bool follow_context_chain = (flags & FOLLOW_CONTEXT_CHAIN) != 0;
-  bool failed_whitelist = false;
   *index = kNotFound;
   *attributes = ABSENT;
   *init_flag = kCreatedInitialized;
@@ -357,6 +356,17 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
           return extension;
         }
       }
+
+      // Check blacklist. Names that are listed, cannot be resolved further.
+      Object blacklist = context->get(BLACK_LIST_INDEX);
+      if (blacklist.IsStringSet() &&
+          StringSet::cast(blacklist).Has(isolate, name)) {
+        if (FLAG_trace_contexts) {
+          PrintF(" - name is blacklisted. Aborting.\n");
+        }
+        break;
+      }
+
       // Check the original context, but do not follow its context chain.
       Object obj = context->get(WRAPPED_CONTEXT_INDEX);
       if (obj.IsContext()) {
@@ -366,26 +376,12 @@ Handle<Object> Context::Lookup(Handle<Context> context, Handle<String> name,
                             attributes, init_flag, variable_mode);
         if (!result.is_null()) return result;
       }
-      // Check whitelist. Names that do not pass whitelist shall only resolve
-      // to with, script or native contexts up the context chain.
-      obj = context->get(WHITE_LIST_INDEX);
-      if (obj.IsStringSet()) {
-        failed_whitelist =
-            failed_whitelist || !StringSet::cast(obj).Has(isolate, name);
-      }
     }
 
     // 3. Prepare to continue with the previous (next outermost) context.
     if (context->IsNativeContext()) break;
 
-    do {
-      context = Handle<Context>(context->previous(), isolate);
-      // If we come across a whitelist context, and the name is not
-      // whitelisted, then only consider with, script, module or native
-      // contexts.
-    } while (failed_whitelist && !context->IsScriptContext() &&
-             !context->IsNativeContext() && !context->IsWithContext() &&
-             !context->IsModuleContext());
+    context = Handle<Context>(context->previous(), isolate);
   } while (follow_context_chain);
 
   if (FLAG_trace_contexts) {

src/objects/contexts.h

@@ -518,7 +518,7 @@ class Context : public HeapObject {
 
     // These slots hold values in debug evaluate contexts.
     WRAPPED_CONTEXT_INDEX = MIN_CONTEXT_SLOTS,
-    WHITE_LIST_INDEX = MIN_CONTEXT_SLOTS + 1
+    BLACK_LIST_INDEX = MIN_CONTEXT_SLOTS + 1
   };
 
   // A region of native context entries containing maps for functions created

test/debugger/debug/debug-evaluate-shadowed-context-3.js

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Test that debug-evaluate properly shadows stack-allocated variables.
+
+Debug = debug.Debug
+
+let exception = null;
+function listener(event, exec_state, event_data, data) {
+  if (event != Debug.DebugEvent.Break) return;
+  try {
+    assertEquals(2, exec_state.frame(0).evaluate("b").value());
+    assertEquals(3, exec_state.frame(0).evaluate("c").value())
+    assertThrows(() => exec_state.frame(0).evaluate("a").value());
+  } catch (e) {
+    exception = e;
+    print(e + e.stack);
+  }
+}
+
+Debug.setListener(listener);
+
+(function f() {
+  let a = 1;
+  let b = 2;
+  let c = 3;
+  () => a + c;  // a and c are context-allocated
+  return function g() {
+    let a = 2;  // a is stack-allocated
+    return function h() {
+      b;  // b is allocated onto f's context.
+      debugger;
+    }
+  }
+})()()();
+
+Debug.setListener(null);
+assertNull(exception);