mac: Fix out-of-bounds access in GetAlteredLibraryData.

With a chance of 1/256, we'd read past the end of kValidLiteralValues.
In practice, that read would return zeroes, which we'd write to
kTestLibData -- but zeroes aren't valid literal values, so that would
then make TestShader() crash when it tried to link the program.

Not sure if this is related to crbug.com/1051923, but it's in the
same area at least.

Add a components/metal_util:unit_tests target. Covering code is useful
even if the test doesn't verify a lot, because then the ASan bots can
find stuff like this for us. (...but in this case, asan doesn't find
the out-of-bounds read because we build with `-asan-globals=0` on
macOS because of https://crbug.com/352073 .
But the test also checks for non-0 and runs the code often enough
that the test consistently fails before the fix and consistently passes
with it.)

Drive-by no-op cleanups while here:
- Remove EXPORTs on enums, they don't have any effect
- Remove redundant is_ios block in components/BUILD.gn

Bug: 1147027,1051923,1085899
Cq-Include-Trybots: luci.chromium.try:mac_chromium_asan_rel_ng
Change-Id: Ia7f9735e450c70eb64b826a499dc60690714112e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2526161
Auto-Submit: Nico Weber <thakis@chromium.org>
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#825730}

components/BUILD.gn

@@ -224,7 +224,7 @@ test("components_unittests") {
       "//components/translate/ios/browser:unit_tests",
       "//components/ukm/ios:unit_tests",
     ]
-  } else {  # !iOS
+  } else {  # !is_ios
     deps += [
       "//components/autofill/content/browser:unit_tests",
       "//components/autofill/content/renderer:unit_tests",
@@ -284,6 +284,7 @@ test("components_unittests") {
       "//components/safe_browsing/content/triggers:unit_tests",
       "//components/safe_browsing/content/web_ui:unit_tests",
       "//components/safe_browsing/core/common:unit_tests",
+      "//components/safe_browsing/core/realtime:unit_tests",
       "//components/safe_browsing/core/triggers:unit_tests",
       "//components/safety_check:unit_tests",
       "//components/security_interstitials/content:unit_tests",
@@ -306,6 +307,8 @@ test("components_unittests") {
       "//components/web_cache/browser:unit_tests",
       "//components/web_package:unit_tests",
       "//components/webcrypto:unit_tests",
+      "//components/webrtc_logging/browser:unit_tests",
+      "//components/webrtc_logging/common:unit_tests",
     ]
 
     if (!is_win) {  # !iOS and !Windows
@@ -415,6 +418,10 @@ test("components_unittests") {
     ]
   }
 
+  if (is_mac) {
+    deps += [ "//components/metal_util:unit_tests" ]
+  }
+
   if (toolkit_views) {
     deps += [ "//components/constrained_window:unit_tests" ]
   }
@@ -452,14 +459,6 @@ test("components_unittests") {
     deps += [ "//components/safe_browsing/content/renderer/phishing_classifier:unit_tests" ]
   }
 
-  if (!is_ios) {
-    deps += [
-      "//components/safe_browsing/core/realtime:unit_tests",
-      "//components/webrtc_logging/browser:unit_tests",
-      "//components/webrtc_logging/common:unit_tests",
-    ]
-  }
-
   if (use_dbus) {
     deps += [
       "//components/dbus/menu:unit_tests",

components/metal_util/BUILD.gn

@@ -2,6 +2,8 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+assert(is_mac)
+
 import("//testing/test.gni")
 
 component("metal_util") {
@@ -42,3 +44,13 @@ component("metal_util") {
     "MetalKit.framework",
   ]
 }
+
+source_set("unit_tests") {
+  testonly = true
+  sources = [ "test_shader_unittest.cc" ]
+  deps = [
+    ":metal_util",
+    "//base",
+    "//testing/gtest",
+  ]
+}

components/metal_util/test_shader.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_METAL_UTIL_TEST_SHADER_H_
 #define COMPONENTS_METAL_UTIL_TEST_SHADER_H_
 
+#include <vector>
 #include "base/callback.h"
 #include "base/task_runner.h"
 #include "base/time/time.h"
@@ -12,14 +13,14 @@
 
 namespace metal {
 
-enum class METAL_UTIL_EXPORT TestShaderComponent {
+enum class TestShaderComponent {
   // Test a shader compile from source.
   kCompile,
   // Test linking a precompiled shader.
   kLink,
 };
 
-enum class METAL_UTIL_EXPORT TestShaderResult {
+enum class TestShaderResult {
   // Not attempted (e.g, because macOS version does not support Metal).
   kNotAttempted,
   // Shader compile succeeded.
@@ -55,10 +56,16 @@ constexpr base::TimeDelta kTestShaderDelay = base::TimeDelta::FromMinutes(3);
 // that |callback| will be called either on another thread or inside the
 // TestShader function call.
 // https://crbug.com/974219
-void METAL_UTIL_EXPORT
-TestShader(TestShaderCallback callback,
-           const base::TimeDelta& delay = kTestShaderDelay,
-           const base::TimeDelta& timeout = kTestShaderTimeForever);
+METAL_UTIL_EXPORT void TestShader(
+    TestShaderCallback callback,
+    const base::TimeDelta& delay = kTestShaderDelay,
+    const base::TimeDelta& timeout = kTestShaderTimeForever);
+
+// Exposed for testing.
+METAL_UTIL_EXPORT extern const size_t kTestLibSize;
+METAL_UTIL_EXPORT extern const size_t kLiteralOffset;
+METAL_UTIL_EXPORT extern const size_t kLiteralSize;
+METAL_UTIL_EXPORT std::vector<uint8_t> GetAlteredLibraryData();
 
 }  // namespace metal
 

components/metal_util/test_shader.mm

@@ -50,8 +50,11 @@ const char* kTestShaderSource =
     "    return %f * in.color;\n"
     "}\n"
     "";
+}  // namesspace
 
-size_t kTestLibSize = 0x1766;
+const size_t kTestLibSize = 0x1766;
+
+namespace {
 uint8_t kTestLibData[] = {
     0x4d, 0x54, 0x4c, 0x42, 0x01, 0x80, 0x02, 0x00, 0x03, 0x00, 0x00, 0x00,
     0x00, 0x00, 0x00, 0x00, 0x66, 0x17, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -553,11 +556,14 @@ uint8_t kTestLibData[] = {
     0x70, 0x6c, 0x65, 0x2d, 0x6d, 0x61, 0x63, 0x6f, 0x73, 0x78, 0x31, 0x30,
     0x2e, 0x31, 0x34, 0x2e, 0x30, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
     0x00, 0x00};
+}  // namespace
 
 // There exists a 4-byte literal at this offset, which can be changed to any
 // value in kValidLiteral to defeat caching.
 const size_t kLiteralOffset = 0x167c;
 const size_t kLiteralSize = 0x4;
+
+namespace {
 const uint8_t kValidLiteralValues[] = {
     0xef, 0x4c, 0x9a, 0x68, 0xef, 0x4c, 0x9a, 0x70, 0x66, 0x6f, 0xa6, 0x74,
     0xef, 0x4c, 0x9a, 0x78, 0x2a, 0x5e, 0x9f, 0x7a, 0x66, 0x6f, 0xa6, 0x7c,
@@ -658,12 +664,15 @@ crash_reporter::CrashKeyString<32>& GetLinkIndexCrashKey() {
   return crash_key;
 }
 
+}  // namespace
+
 std::vector<uint8_t> GetAlteredLibraryData() {
   // Make a copy of the shader's data.
   std::vector<uint8_t> data(kTestLibData, kTestLibData + kTestLibSize);
 
   // Alter the data at kLiteralOffset to defeat caching.
-  uint64_t index = base::RandInt(0, sizeof(kValidLiteralValues) / kLiteralSize);
+  uint64_t index =
+      base::RandInt(0, sizeof(kValidLiteralValues) / kLiteralSize - 1);
   for (size_t i = 0; i < kLiteralSize; ++i)
     data[kLiteralOffset + i] = kValidLiteralValues[kLiteralSize * index + i];
 
@@ -677,6 +686,8 @@ std::vector<uint8_t> GetAlteredLibraryData() {
   return data;
 }
 
+namespace {
+
 base::ScopedDispatchObject<dispatch_data_t> GetLibraryData() {
   auto vector_data = GetAlteredLibraryData();
   base::ScopedDispatchObject<dispatch_data_t> dispatch_data(

components/metal_util/test_shader_unittest.cc

+// Copyright 2020 The Chromium 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 "components/metal_util/test_shader.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+TEST(GetAlteredLibraryDataTest, TestInvalidAccess) {
+  for (int j = 0; j < 4096; ++j) {
+    std::vector<uint8_t> data = metal::GetAlteredLibraryData();
+    EXPECT_EQ(data.size(), metal::kTestLibSize);
+    for (size_t i = 0; i < metal::kLiteralSize; ++i)
+      EXPECT_NE(data[metal::kLiteralOffset + i], 0);
+  }
+}
+
+}  // namespace