[gcmole] Enable use-after-free detection

GCMole now comes with the long forgotten use-after-free detection
enabled by default. The CL also improves error logging when test
expectations mismatch with the actual output and updates the hash
of GCMole to be used with the newly built version with enabled UAF
detection.

The CL also contains an ignore for isolate.cc due to inability to
fix a warning there and fixes a couple of UAF warnings.

Bug: v8:9680
Change-Id: I7a009ffd5f67b1b5437567691ca4235ea873de70
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2257236
Commit-Queue: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Cr-Commit-Position: refs/heads/master@{#68505}

src/runtime/runtime-wasm.cc

@@ -457,14 +457,13 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
   // Enter the debugger.
   DebugScope debug_scope(isolate->debug());
 
-  const auto undefined = ReadOnlyRoots(isolate).undefined_value();
   WasmFrame* frame = frame_finder.frame();
   auto* debug_info = frame->native_module()->GetDebugInfo();
   if (debug_info->IsStepping(frame)) {
     debug_info->ClearStepping(isolate);
     isolate->debug()->ClearStepping();
     isolate->debug()->OnDebugBreak(isolate->factory()->empty_fixed_array());
-    return undefined;
+    return ReadOnlyRoots(isolate).undefined_value();
   }
 
   // Check whether we hit a breakpoint.
@@ -491,7 +490,7 @@ RUNTIME_FUNCTION(Runtime_WasmDebugBreak) {
     debug_info->RemoveBreakpoint(frame->function_index(), position, isolate);
   }
 
-  return undefined;
+  return ReadOnlyRoots(isolate).undefined_value();
 }
 
 }  // namespace internal

src/wasm/wasm-debug.cc

@@ -51,14 +51,14 @@ Handle<String> PrintFToOneByteString(Isolate* isolate, const char* format,
 MaybeHandle<JSObject> CreateFunctionTablesObject(
     Handle<WasmInstanceObject> instance) {
   Isolate* isolate = instance->GetIsolate();
-  auto tables = instance->tables();
-  if (tables.length() == 0) return MaybeHandle<JSObject>();
+  auto tables = handle(instance->tables(), isolate);
+  if (tables->length() == 0) return MaybeHandle<JSObject>();
 
   const char* table_label = "table%d";
   Handle<JSObject> tables_obj = isolate->factory()->NewJSObjectWithNullProto();
-  for (int table_index = 0; table_index < tables.length(); ++table_index) {
+  for (int table_index = 0; table_index < tables->length(); ++table_index) {
     auto func_table =
-        handle(WasmTableObject::cast(tables.get(table_index)), isolate);
+        handle(WasmTableObject::cast(tables->get(table_index)), isolate);
     if (func_table->type().heap_type() != kHeapFunc) continue;
 
     Handle<String> table_name;

tools/gcmole/gcmole-test.cc

@@ -189,7 +189,7 @@ void TestGuardedDeadVarAnalysisNotOnStack(Isolate* isolate) {
 void TestGuardedDeadVarAnalysisNested(JSObject raw_obj, Isolate* isolate) {
   CauseGCRaw(raw_obj, isolate);
 
-  // Shouldn't cause warning.
+  // Should cause warning.
   raw_obj.Print();
 }
 
@@ -198,6 +198,9 @@ void TestGuardedDeadVarAnalysisCaller(Isolate* isolate) {
   JSObject raw_obj = *isolate->factory()->NewJSObjectWithNullProto();
 
   TestGuardedDeadVarAnalysisNested(raw_obj, isolate);
+
+  // Shouldn't cause warning.
+  raw_obj.Print();
 }
 
 JSObject GuardedAllocation(Isolate* isolate) {

tools/gcmole/gcmole-tools.tar.gz.sha1

-d2f949820bf1ee7343a7b5f46987a3657aaea2e9
+0af04ef475bc746a501fe17d3b56ccb03fc151fc
\ No newline at end of file

tools/gcmole/gcmole.cc

@@ -66,7 +66,7 @@ bool g_dead_vars_analysis = false;
 // to provide extra info for the GC suspect.
 #define TRACE_LLVM_DECL(str, decl)                   \
   do {                                               \
-    if (g_dead_vars_analysis) {      \
+    if (g_tracing_enabled && g_dead_vars_analysis) { \
       std::cout << str << std::endl;                 \
       decl->dump();                                  \
     }                                                \
@@ -368,6 +368,11 @@ static bool KnownToCauseGC(clang::MangleContext* ctx,
 
   if (!InV8Namespace(decl)) return false;
 
+  if (suspects_whitelist.find(decl->getNameAsString()) !=
+      suspects_whitelist.end()) {
+    return false;
+  }
+
   MangledName name;
   if (GetMangledName(ctx, decl, &name)) {
     return gc_suspects.find(name) != gc_suspects.end();
@@ -1358,15 +1363,6 @@ class FunctionAnalyzer {
   }
 
   bool IsInternalPointerType(clang::QualType qtype) {
-    // Not yet assigned pointers can't get moved by the GC.
-    if (qtype.isNull()) {
-      return false;
-    }
-    // nullptr can't get moved by the GC.
-    if (qtype->isNullPtrType()) {
-      return false;
-    }
-
     const clang::CXXRecordDecl* record = qtype->getAsCXXRecordDecl();
     bool result = IsDerivedFromInternalPointer(record);
     TRACE_LLVM_TYPE("is internal " << result, qtype);
@@ -1376,6 +1372,15 @@ class FunctionAnalyzer {
   // Returns weather the given type is a raw pointer or a wrapper around
   // such. For V8 that means Object and MaybeObject instances.
   bool RepresentsRawPointerType(clang::QualType qtype) {
+    // Not yet assigned pointers can't get moved by the GC.
+    if (qtype.isNull()) {
+      return false;
+    }
+    // nullptr can't get moved by the GC.
+    if (qtype->isNullPtrType()) {
+      return false;
+    }
+
     const clang::PointerType* pointer_type =
         llvm::dyn_cast_or_null<clang::PointerType>(qtype.getTypePtrOrNull());
     if (pointer_type != NULL) {

tools/gcmole/gcmole.lua

@@ -40,9 +40,8 @@ local FLAGS = {
    -- Print commands to console before executing them.
    verbose = false;
 
-   -- Perform dead variable analysis (generates many false positives).
-   -- TODO add some sort of whiteliste to filter out false positives.
-   dead_vars = false;
+   -- Perform dead variable analysis.
+   dead_vars = true;
 
    -- Enable verbose tracing from the plugin itself.
    verbose_trace = false;
@@ -322,6 +321,7 @@ local WHITELIST = {
 
    -- CodeCreateEvent receives AbstractCode (a raw ptr) as an argument.
    "CodeCreateEvent",
+   "WriteField",
 };
 
 local function AddCause(name, cause)
@@ -477,6 +477,17 @@ local function SafeCheckCorrectnessForArch(arch, for_test)
    return errors, output
 end
 
+-- Source: https://stackoverflow.com/a/41515925/1540248
+local function StringDifference(str1,str2)
+   for i = 1,#str1 do -- Loop over strings
+         -- If that character is not equal to its counterpart
+         if str1:sub(i,i) ~= str2:sub(i,i) then
+            return i --Return that index
+         end
+   end
+   return #str1+1 -- Return the index after where the shorter one ends as fallback.
+end
+
 local function TestRun()
    local errors, output = SafeCheckCorrectnessForArch('x64', true)
    if not errors then
@@ -491,6 +502,16 @@ local function TestRun()
 
    if output ~= expectations then
       log("** Output mismatch from running tests. Please run them manually.")
+      local idx = StringDifference(output, expectations)
+
+      log("Difference at byte "..idx)
+      log("Expected: "..expectations:sub(idx-10,idx+10))
+      log("Actual: "..output:sub(idx-10,idx+10))
+
+      log("--- Full output ---")
+      log(output)
+      log("------")
+
       return false
    end
 

tools/gcmole/ignored_files

 src/profiler/heap-snapshot-generator.cc
+src/execution/isolate.cc

tools/gcmole/test-expectations.txt

 
+tools/gcmole/gcmole-test.cc:27:10: warning: Possibly dead variable.
+  return obj;
+         ^
 tools/gcmole/gcmole-test.cc:45:3: warning: Possible problem with evaluation order.
   TwoArgumentsFunction(*CauseGC(obj1, isolate), *CauseGC(obj2, isolate));
   ^
@@ -20,4 +23,13 @@ tools/gcmole/gcmole-test.cc:130:14: warning: Possible problem with evaluation or
 tools/gcmole/gcmole-test.cc:151:14: warning: Possible problem with evaluation order.
   so_handle->Method(*SomeClass::StaticCauseGC(obj1, isolate));
              ^
-7 warnings generated.
+tools/gcmole/gcmole-test.cc:161:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:193:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+tools/gcmole/gcmole-test.cc:216:3: warning: Possibly dead variable.
+  raw_obj.Print();
+  ^
+11 warnings generated.