Fix possible crash in SafeStackFrameIterator

src/frames.cc

 // Copyright 2012 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.
 
 #include "src/frames.h"
 
 #include <sstream>
 
 #include "src/ast/ast.h"
 #include "src/ast/scopeinfo.h"
@@ skipping 103 matching lines @@
   if (type == StackFrame::NONE) return NULL;
   StackFrame* result = SingletonFor(type);
   DCHECK(result != NULL);
   result->state_ = *state;
   return result;
 }
 
 
 StackFrame* StackFrameIteratorBase::SingletonFor(StackFrame::Type type) {
 #define FRAME_TYPE_CASE(type, field) \
-  case StackFrame::type: result = &field##_; break;
+  case StackFrame::type:             \
+    return &field##_;
 
-  StackFrame* result = NULL;
   switch (type) {
     case StackFrame::NONE: return NULL;
     STACK_FRAME_TYPE_LIST(FRAME_TYPE_CASE)
     default: break;
   }
-  return result;
+  return NULL;
 
 #undef FRAME_TYPE_CASE
 }
 
 
 // -------------------------------------------------------------------------
 
 
 JavaScriptFrameIterator::JavaScriptFrameIterator(
     Isolate* isolate, StackFrame::Id id)
@@ skipping 84 matching lines @@
       // The frame anyways will be skipped.
       type = StackFrame::JAVA_SCRIPT;
       // Top frame is incomplete so we cannot reliably determine its type.
       top_frame_type_ = StackFrame::NONE;
     }
   } else {
     return;
   }
   if (SingletonFor(type) == NULL) return;
   frame_ = SingletonFor(type, &state);
-  if (frame_ == NULL) return;
+  DCHECK(frame_);

[titzer, 2016/01/27 08:29:00]

Did you run this through the trybots yet? I might have hit this one last time
when adding support for WASM frames. Is it safe(r) to just return if the frame
type is not known?

[alph, 2016/01/27 17:37:14]

AFAICT it cannot return NULL if the previous call was not null.
Dry run passed. I'll try to land it and keep watching it.

 
   Advance();
 
   if (frame_ != NULL && !frame_->is_exit() &&
       external_callback_scope_ != NULL &&
       external_callback_scope_->scope_address() < frame_->fp()) {
     // Skip top ExternalCallbackScope if we already advanced to a JS frame
     // under it. Sampler will anyways take this top external callback.
     external_callback_scope_ = external_callback_scope_->previous();
   }
@@ skipping 17 matching lines @@
   Address last_sp = last_frame->sp(), last_fp = last_frame->fp();
   // Before advancing to the next stack frame, perform pointer validity tests.
   if (!IsValidFrame(last_frame) || !IsValidCaller(last_frame)) {
     frame_ = NULL;
     return;
   }
 
   // Advance to the previous frame.
   StackFrame::State state;
   StackFrame::Type type = frame_->GetCallerState(&state);
+  if (SingletonFor(type) == NULL) {

[yurys, 2016/01/26 23:55:57]

Maybe if (type < 0 || type >= StackFrame::NUMBER_OF_TYPES) ?

[alph, 2016/01/27 00:19:36]

I like mine more. Moreover it is used elsewhere (line 235).

[yurys, 2016/01/27 00:22:14]

Acknowledged.

+    frame_ = NULL;
+    return;
+  }
   frame_ = SingletonFor(type, &state);
-  if (frame_ == NULL) return;
+  DCHECK(frame_);
 
   // Check that we have actually moved to the previous frame in the stack.
   if (frame_->sp() < last_sp || frame_->fp() < last_fp) {
     frame_ = NULL;
   }
 }
 
 
 bool SafeStackFrameIterator::IsValidFrame(StackFrame* frame) const {
   return IsValidStackAddress(frame->sp()) && IsValidStackAddress(frame->fp());
@@ skipping 1329 matching lines @@
   for (StackFrameIterator it(isolate); !it.done(); it.Advance()) {
     StackFrame* frame = AllocateFrameCopy(it.frame(), zone);
     list.Add(frame, zone);
   }
   return list.ToVector();
 }
 
 
 }  // namespace internal
 }  // namespace v8