Unsafe-buffer error: Handle duplicates.

Handle duplicates in opt-in/opt-out path prefixes to avoid unexpected
behavior.

This change normalizes the opt-in and opt-out path prefixes by sorting
and removing duplicates or sub-prefixes. This ensures that adding
additional opt-outs, even if redundant, does not inadvertently opt-in
other targets. This is important, especially when landing concurrent
patches, or revert/relanding some of them.

Bug: 357649176
Change-Id: I253b38ff330d33e5053703ecafe178de91471844
Fixed: 357649176
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5796896
Commit-Queue: Arthur Sonzogni <[email protected]>
Reviewed-by: Daniel Cheng <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1344630}

tools/clang/plugins/UnsafeBuffersPlugin.cpp

@@ -29,6 +29,35 @@ struct CheckFilePrefixes {
   std::vector<llvm::StringRef> opt_in;
 };
 
+// Sort the prefixes and remove duplicates.
+void NormalizePrefixList(std::vector<llvm::StringRef>& prefixes) {
+  // TODO(danakj): Use std::ranges::sort when Clang is build with C++20.
+  std::sort(prefixes.begin(), prefixes.end());
+
+  // Remove ~duplicate in a general sense, where a prefix is a prefix of another
+  // prefix. This is useful when two unrelated patches are merged/reverted
+  // around the same time and the prefixes overlap. This avoids one to hide the
+  // other when searching them via std::upper_bound.
+  //
+  // Note that we could use std::unique here, but its behavior is not
+  // guaranteed by the standard when the predicate is not an equivalence
+  // relation.
+  {
+    auto it = prefixes.begin();
+    for (auto next = std::next(it); next != prefixes.end(); ++next) {
+      if (next->starts_with(*it)) {
+        continue;  // Skip the prefix.
+      }
+      ++it;
+      // Fill in the gap in between the two iterators.
+      if (it != next) {
+        *it = std::move(*next);
+      }
+    }
+    prefixes.erase(std::next(it), prefixes.end());
+  }
+}
+
 class UnsafeBuffersDiagnosticConsumer : public clang::DiagnosticConsumer {
  public:
   UnsafeBuffersDiagnosticConsumer(clang::DiagnosticsEngine* engine,
@@ -421,11 +450,9 @@ class UnsafeBuffersASTAction : public clang::PluginASTAction {
       }
     }
 
-    // TODO(danakj): Use std::ranges::sort when Clang is build with C++20.
-    std::sort(check_file_prefixes_.opt_in.begin(),
-              check_file_prefixes_.opt_in.end());
-    std::sort(check_file_prefixes_.opt_out.begin(),
-              check_file_prefixes_.opt_out.end());
+    NormalizePrefixList(check_file_prefixes_.opt_in);
+    NormalizePrefixList(check_file_prefixes_.opt_out);
+
     return true;
   }
 

tools/clang/plugins/tests/unsafe_buffers_not_clean_dir/still_not_clean_dir_1/not_clean_header.h

@@ -0,0 +1,11 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef TOOLS_CLANG_PLUGINS_TESTS_UNSAFE_BUFFERS_NOT_CLEAN_DIR_STILL_NOT_CLEAN_DIR_1_NOT_CLEAN_HEADER_H_
+#define TOOLS_CLANG_PLUGINS_TESTS_UNSAFE_BUFFERS_NOT_CLEAN_DIR_STILL_NOT_CLEAN_DIR_1_NOT_CLEAN_HEADER_H_
+
+inline int unchecked_violation_1(int* i, unsigned s) {
+  return i[s];  // This is in a known-bad header, so no error is emitted.
+}
+
+#endif  // TOOLS_CLANG_PLUGINS_TESTS_UNSAFE_BUFFERS_NOT_CLEAN_DIR_STILL_NOT_CLEAN_DIR_1_NOT_CLEAN_HEADER_H_

tools/clang/plugins/tests/unsafe_buffers_not_clean_dir/still_not_clean_dir_2/not_clean_header.h

@@ -0,0 +1,11 @@
+// Copyright 2024 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+#ifndef TOOLS_CLANG_PLUGINS_TESTS_UNSAFE_BUFFERS_NOT_CLEAN_DIR_STILL_NOT_CLEAN_DIR_2_NOT_CLEAN_HEADER_H_
+#define TOOLS_CLANG_PLUGINS_TESTS_UNSAFE_BUFFERS_NOT_CLEAN_DIR_STILL_NOT_CLEAN_DIR_2_NOT_CLEAN_HEADER_H_
+
+inline int unchecked_violation_2(int* i, unsigned s) {
+  return i[s];  // This is in a known-bad header, so no error is emitted.
+}
+
+#endif  // TOOLS_CLANG_PLUGINS_TESTS_UNSAFE_BUFFERS_NOT_CLEAN_DIR_STILL_NOT_CLEAN_DIR_2_NOT_CLEAN_HEADER_H_

tools/clang/plugins/tests/unsafe_buffers_paths.txt

@@ -6,3 +6,7 @@
 
 -unsafe_buffers_unchecked.cpp
 
+# Unnecessary exclusion of a directory that is already excluded.
+# This shouldn't produce surprising behaviors, like including
+# `unsafe_buffers_not_clean_dir_2/` again. See https://crbug.com/357649176
+-unsafe_buffers_not_clean_dir/still_not_clean_dir_1/

tools/clang/plugins/tests/unsafe_buffers_unchecked.cpp

@@ -13,6 +13,8 @@
 
 #include "unsafe_buffers_not_clean_dir/clean_header.h"
 #include "unsafe_buffers_not_clean_dir/not_clean_header.h"
+#include "unsafe_buffers_not_clean_dir/still_not_clean_dir_1/not_clean_header.h"
+#include "unsafe_buffers_not_clean_dir/still_not_clean_dir_2/not_clean_header.h"
 
 // This is in a known-bad cc file, so no error is emitted.
 DO_UNSAFE_THING_FROM_CHECKED_HEADER(UncheckedCpp, N, i, s);    // No error.