Fix use-after-free bug in debugger

runtime/vm/debugger.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/debugger.h"
 
 #include "vm/code_index_table.h"
 #include "vm/code_generator.h"
 #include "vm/code_patcher.h"
 #include "vm/compiler.h"
@@ skipping 299 matching lines @@
   ASSERT(!func.HasOptimizedCode());
   Code& code = Code::Handle(func.unoptimized_code());
   ASSERT(!code.IsNull());  // Function must be compiled.
   PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
   ASSERT(pc_desc_index < desc.Length());
   token_index_ = desc.TokenIndex(pc_desc_index);
   ASSERT(token_index_ >= 0);
   pc_ = desc.PC(pc_desc_index);
   ASSERT(pc_ != 0);
   breakpoint_kind_ = desc.DescriptorKind(pc_desc_index);
+  ASSERT((breakpoint_kind_ == PcDescriptors::kIcCall) ||
+         (breakpoint_kind_ == PcDescriptors::kFuncCall) ||
+         (breakpoint_kind_ == PcDescriptors::kReturn));
 }
 
 
 CodeBreakpoint::~CodeBreakpoint() {
   // Make sure we don't leave patched code behind.
   ASSERT(!IsEnabled());
+  // Poison the data so we catch use after free errors.
+#ifdef DEBUG
+  function_ = Function::null();
+  pc_ = 0ul;
+  src_bpt_ = NULL;
+  next_ = NULL;
+  breakpoint_kind_ = PcDescriptors::kOther;
+#endif
 }
 
 
 RawScript* CodeBreakpoint::SourceCode() {
   const Function& func = Function::Handle(function_);
   const Class& cls = Class::Handle(func.owner());
   return cls.script();
 }
 
 
@@ skipping 171 matching lines @@
 }
 
 
 void Debugger::InstrumentForStepping(const Function& target_function) {
   if (target_function.HasCode()) {
     EnsureFunctionIsDeoptimized(target_function);
   } else {
     Compiler::CompileFunction(target_function);
     // If there were any errors, ignore them silently and return without
     // adding breakpoints to target.
     if (!target_function.HasCode()) {

[siva, 2012/03/19 22:45:22]

If this path is taken internal breakpoints would not be deleted. 
Maybe there should be a RemoveInternalBreakpoints() here too.

[hausner, 2012/03/19 23:35:15]

See comments below.

       return;
     }
   }
+  RemoveInternalBreakpoints();
   Code& code = Code::Handle(target_function.unoptimized_code());
   ASSERT(!code.IsNull());
   PcDescriptors& desc = PcDescriptors::Handle(code.pc_descriptors());
   for (int i = 0; i < desc.Length(); i++) {
     CodeBreakpoint* bpt = GetCodeBreakpoint(desc.PC(i));
     if (bpt != NULL) {
       // There is already a breakpoint for this address. Leave it alone.
       continue;
     }
     PcDescriptors::Kind kind = desc.DescriptorKind(i);
@@ skipping 342 matching lines @@
 
   if (resume_action_ == kContinue) {
     RemoveInternalBreakpoints();
   } else if (resume_action_ == kStepOver) {
     Function& func = Function::Handle(bpt->function());
     if (bpt->breakpoint_kind_ == PcDescriptors::kReturn) {
       // If we are at the function return, do a StepOut action.
       if (stack_trace->Length() > 1) {
         ActivationFrame* caller = stack_trace->ActivationFrameAt(1);
         func = caller->DartFunction().raw();
-        RemoveInternalBreakpoints();
       }
     }
     InstrumentForStepping(func);
   } else if (resume_action_ == kStepInto) {
-    RemoveInternalBreakpoints();
     if (bpt->breakpoint_kind_ == PcDescriptors::kIcCall) {
       int num_args, num_named_args;
       uword target;
       CodePatcher::GetInstanceCallAt(bpt->pc_, NULL,
           &num_args, &num_named_args, &target);
       ActivationFrame* top_frame = stack_trace->ActivationFrameAt(0);
       Instance& receiver = Instance::Handle(
           top_frame->GetInstanceCallReceiver(num_args));
       Code& code = Code::Handle(
           ResolveCompileInstanceCallTarget(isolate_, receiver));
       if (!code.IsNull()) {
         Function& callee = Function::Handle(code.function());
         InstrumentForStepping(callee);
       }

[siva, 2012/03/19 22:45:22]

What happens if code.IsNull() is true? (internal breakpoints not removed).

[hausner, 2012/03/19 23:35:15]

Ok, mixing the instrumenting function and deleting temp breakpoints was a bad
idea. I added explicit calls again here.

     } else if (bpt->breakpoint_kind_ == PcDescriptors::kFuncCall) {
       Function& callee = Function::Handle();
       uword target;
       CodePatcher::GetStaticCallAt(bpt->pc_, &callee, &target);
       InstrumentForStepping(callee);
     } else {
       ASSERT(bpt->breakpoint_kind_ == PcDescriptors::kReturn);
       // Treat like stepping out to caller.
       if (stack_trace->Length() > 1) {
         ActivationFrame* caller = stack_trace->ActivationFrameAt(1);
         InstrumentForStepping(caller->DartFunction());
       }

[siva, 2012/03/19 22:45:22]

What about the case when stack_trace->Length() == 1? (internal breakpoints not
removed).

[hausner, 2012/03/19 23:35:15]

Ditto.

     }
   } else {
     ASSERT(resume_action_ == kStepOut);
     // Set stepping breakpoints in the caller.
-    RemoveInternalBreakpoints();
     if (stack_trace->Length() > 1) {
       ActivationFrame* caller = stack_trace->ActivationFrameAt(1);
       InstrumentForStepping(caller->DartFunction());

[siva, 2012/03/19 22:45:22]

Ditto question.

[hausner, 2012/03/19 23:35:15]

Ditto.

     }
   }
 }
 
 
 void Debugger::Initialize(Isolate* isolate) {
   if (initialized_) {
     return;
   }
   isolate_ = isolate;
@@ skipping 138 matching lines @@
 }
 
 
 void Debugger::RegisterCodeBreakpoint(CodeBreakpoint* bpt) {
   ASSERT(bpt->next() == NULL);
   bpt->set_next(code_breakpoints_);
   code_breakpoints_ = bpt;
 }
 
 }  // namespace dart