0

Reland "Use a clang plugin to emit and filter -Werror=unsafe-buffer-usage"

This is a reland of commit a7fa6958b2

This reland avoids adding the unsafe_buffers_paths.txt file as an
input to GN targets that are not C/C++.

Original change's description:
> Use a clang plugin to emit and filter -Werror=unsafe-buffer-usage
>
> Part 2 of 2 for "Allow incremental adoption of -Wunsafe-buffer-usage
> at a per-file (either header or source file) granularity."
>
> A key difficulty of spanification is that of incremental application
> of the -Wunsafe-buffer-usage warning:
> * First, textual includes mean that it's not possible to turn on the
>   warning for a header file only. It has to be enabled for a cc file
>   at a time.
> * Then, enabling it for a cc file also transitively applies it to every
>   header it touches.
> * And, enabling it on separate cc files requires splitting up our GN
>   build targets quite awkwardly, and will require a huge amount of
>   churn.
>
> Clang itself does not provide a way to specify a warning should be
> enabled only for certain files (crbug.com/41497066).
>
> In https://chromium-review.googlesource.com/c/chromium/src/+/5323341
> we introduced the 'unsafe-buffers' plugin, which is able to apply
> -Wunsafe-buffer-usage at the individual file level. In this CL, we
> enable use of the plugin, and remove other existing mechanisms.
>
> We add a list of checked file-path prefixes in
> //build/config/unsafe_buffers_paths.txt which are read by the plugin
> so that individual headers, cc files, and eventually directories
> can have the warning enabled for them in an incremental manner
> without requiring changes to the plugin or clang rolls.
>
> In order to rebuild targets when the paths change, the path prefixes
> file is added to the `inputs` set in GN for every C/C++ target.
> However Reclient does not respect `inputs` for C/C++. It must for
> Rust, to support `include!()`, and it will need to in the future to
> support `#embed` in C. A workaround is added for this file in
> //build/config/rbe.gni that just sticks the file in the command
> line to rewrapped explicitly, passing it as `-inputs`. See
> crbug.com/326584510 for this Reclient bug.
>
> We remove the UNSAFE_BUFFERS_INCLUDE_BEGIN and
> UNSAFE_BUFFERS_INCLUDE_END macros which were used to wrap header
> includes and exclude them from unsafe-buffers checks as they
> no longer provide additional value. Headers will only be checked
> if they are opted in through the unsafe-buffers plugin.
>
> And we remove the -Wunsafe-buffer-usage command line argument
> from cflags as we use the plugin to do this job now. One day when
> the codebase is clean, we can turn this warning on globally. The
> dream.
>
> Bug: 40284755, 41497066, 326584510
> Change-Id: I03c43c2a6ab5552f58c6fcd0cb885ad2ce1460d7
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5324046
> Commit-Queue: danakj <danakj@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1267242}

Bug: 40284755, 41497066, 326584510
Change-Id: I1023be17b36ac05dfc5806ce14ff745c246a0a63
Cq-Include-Trybots: luci.chromium.try:ios-device,ios-simulator
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5340729
Reviewed-by: Hans Wennborg <hans@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1269212}
This commit is contained in:
danakj
2024-03-06 20:47:46 +00:00
committed by Chromium LUCI CQ
parent 5bae00a566
commit 4e625fbecf
15 changed files with 174 additions and 83 deletions

@ -3195,6 +3195,7 @@ test("base_unittests") {
"command_line_unittest.cc",
"component_export_unittest.cc",
"containers/adapters_unittest.cc",
"containers/buffer_iterator_unittest.cc",
"containers/checked_iterators_unittest.cc",
"containers/circular_deque_unittest.cc",
"containers/contains_unittest.cc",
@ -3536,6 +3537,7 @@ test("base_unittests") {
"types/token_type_unittest.cc",
"types/variant_util_unittest.cc",
"unguessable_token_unittest.cc",
"unsafe_buffers_unittest.cc",
"uuid_unittest.cc",
"value_iterators_unittest.cc",
"values_unittest.cc",
@ -3596,9 +3598,6 @@ test("base_unittests") {
"//third_party/icu",
"//third_party/modp_b64",
]
if (is_clang) {
deps += [ ":base_unsafe_buffers_unittest_sources" ]
}
data_deps = [
"//base/test:immediate_crash_test_helper",
@ -4195,26 +4194,9 @@ test("base_unittests") {
if (enable_nocompile_tests) {
deps += [ ":base_nocompile_tests" ]
if (is_clang) {
deps += [ ":base_unsafe_buffers_nocompile_tests" ]
}
}
}
source_set("base_unsafe_buffers_unittest_sources") {
testonly = true
visibility = [ ":*" ]
configs += [ "//build/config/compiler:unsafe_buffer_warning" ]
sources = [
"containers/buffer_iterator_unittest.cc",
"unsafe_buffers_unittest.cc",
]
deps = [
"//base",
"//testing/gtest",
]
}
# Test that CFG is enabled in Rust code.
if (is_win && toolchain_has_rust) {
source_set("rust_cfg_win_test") {
@ -4246,6 +4228,7 @@ if (enable_nocompile_tests) {
"allocator/partition_allocator/src/partition_alloc/pointers/raw_ptr_nocompile.nc",
"allocator/partition_allocator/src/partition_alloc/pointers/raw_ref_nocompile.nc",
"callback_list_nocompile.nc",
"containers/buffer_iterator_nocompile.nc",
"containers/checked_iterators_nocompile.nc",
"containers/contains_nocompile.nc",
"containers/enum_set_nocompile.nc",
@ -4275,18 +4258,13 @@ if (enable_nocompile_tests) {
"traits_bag_nocompile.nc",
"types/pass_key_nocompile.nc",
"types/variant_util_nocompile.nc",
"unsafe_buffers_nocompile.nc",
"values_nocompile.nc",
]
deps = [ ":base" ]
}
nocompile_source_set("base_unsafe_buffers_nocompile_tests") {
configs = [ "//build/config/compiler:unsafe_buffer_warning" ]
sources = [
"containers/buffer_iterator_nocompile.nc",
"unsafe_buffers_nocompile.nc",
deps = [
":base",
"//build/config/clang:unsafe_buffers_buildflags",
]
deps = [ ":base" ]
}
}

@ -502,13 +502,6 @@ inline constexpr bool AnalyzerAssumeTrue(bool arg) {
// explanation requires cooperation of code that is not fully encapsulated close
// to the UNSAFE_BUFFERS() usage, it should be rejected and replaced with safer
// coding patterns or stronger guarantees.
//
// UNSAFE_BUFFERS_INCLUDE_BEGIN / UNSAFE_BUFFERS_INCLUDE_END can be used to
// wrap an `#include` statement for a header which has not been made clean for
// the -Wunsafe-buffer-usage warning. As these disable warnings for the entire
// header, and its transitive dependencies, it's clear these should be temporary
// and the header converted to use safe operations via `span` (or use
// UNSAFE_BUFFERS() explicitly in rare cases).
#if defined(__clang__)
// clang-format off
// Formatting is off so that we can put each _Pragma on its own line, as
@ -518,12 +511,8 @@ inline constexpr bool AnalyzerAssumeTrue(bool arg) {
__VA_ARGS__ \
_Pragma("clang unsafe_buffer_usage end")
// clang-format on
#define UNSAFE_BUFFERS_INCLUDE_BEGIN _Pragma("clang unsafe_buffer_usage begin")
#define UNSAFE_BUFFERS_INCLUDE_END _Pragma("clang unsafe_buffer_usage end")
#else
#define UNSAFE_BUFFERS(...) __VA_ARGS__
#define UNSAFE_BUFFERS_INCLUDE_BEGIN
#define UNSAFE_BUFFERS_INCLUDE_END
#endif
#endif // BASE_COMPILER_SPECIFIC_H_

@ -20,11 +20,7 @@
#include "base/check.h"
#include "base/compiler_specific.h"
// TODO(crbug.com/40284755): checked_iterators should use UNSAFE_BUFFERS()
// internally.
UNSAFE_BUFFERS_INCLUDE_BEGIN
#include "base/containers/checked_iterators.h"
UNSAFE_BUFFERS_INCLUDE_END
#include "base/numerics/safe_conversions.h"
#include "base/template_util.h"
#include "base/types/to_address.h"

@ -7,12 +7,9 @@
#include "base/compiler_specific.h"
// TODO(crbug.com/40284755): raw_ptr should use UNSAFE_BUFFERS() internally.
UNSAFE_BUFFERS_INCLUDE_BEGIN
// Although `raw_ptr` is part of the standalone PA distribution, it is
// easier to use the shorter path in `//base/memory`. We retain this
// facade header for ease of typing.
#include "base/allocator/partition_allocator/src/partition_alloc/pointers/raw_ptr.h" // IWYU pragma: export
UNSAFE_BUFFERS_INCLUDE_END
#endif // BASE_MEMORY_RAW_PTR_H_

@ -7,12 +7,9 @@
#include "base/compiler_specific.h"
// TODO(crbug.com/40284755): raw_ref should use UNSAFE_BUFFERS() internally.
UNSAFE_BUFFERS_INCLUDE_BEGIN
// Although `raw_ref` is part of the standalone PA distribution, it is
// easier to use the shorter path in `//base/memory`. We retain this
// facade header for ease of typing.
#include "base/allocator/partition_allocator/src/partition_alloc/pointers/raw_ref.h" // IWYU pragma: export
UNSAFE_BUFFERS_INCLUDE_END
#endif // BASE_MEMORY_RAW_REF_H_

@ -6,6 +6,7 @@
// http://dev.chromium.org/developers/testing/no-compile-tests
#include "base/compiler_specific.h"
#include "build/config/clang/unsafe_buffers_buildflags.h"
namespace base {
@ -15,7 +16,11 @@ UNSAFE_BUFFER_USAGE int uses_pointer_as_array(int* i) {
void CallToUnsafeBufferFunctionDisallowed() {
int arr[] = {1, 2};
#if BUILDFLAG(UNSAFE_BUFFERS_WARNING_ENABLED)
uses_pointer_as_array(arr); // expected-error {{function introduces unsafe buffer manipulation}}
#else
uses_pointer_as_array(arr); // expected-no-diagnostics: No error when not enabled.
#endif
}
} // namespace base

@ -386,8 +386,9 @@ if (is_android) {
if (is_clang && !is_nacl) {
default_compiler_configs += [
"//build/config/clang:find_bad_constructs",
"//build/config/clang:extra_warnings",
"//build/config/clang:find_bad_constructs",
"//build/config/clang:unsafe_buffers",
]
}
@ -517,6 +518,42 @@ TESTONLY_AND_VISIBILITY = [
"visibility",
]
# Sets default dependencies for static_library and source_set targets.
foreach(_target_type,
[
"source_set",
"static_library",
]) {
template(_target_type) {
target(_target_type, target_name) {
forward_variables_from(invoker, "*", TESTONLY_AND_VISIBILITY)
forward_variables_from(invoker, TESTONLY_AND_VISIBILITY)
if (!defined(inputs)) {
inputs = []
}
# Consumed by the unsafe-buffers plugin during compile.
#
# TODO(crbug.com/326584510): Reclient doesn't respect this variable, see
# rbe_bug_326584510_missing_inputs in //build/config/rbe.gni.
_uses_cflags = false
if (defined(sources)) {
foreach(f, sources) {
if (string_replace(f + ".END", ".cc.END", "") != f + ".END" ||
string_replace(f + ".END", ".c.END", "") != f + ".END" ||
string_replace(f + ".END", ".mm.END", "") != f + ".END" ||
string_replace(f + ".END", ".m.END", "") != f + ".END") {
_uses_cflags = true
}
}
}
if (_uses_cflags) {
inputs += [ "//build/config/unsafe_buffers_paths.txt" ]
}
}
}
}
# Sets default dependencies for executable and shared_library targets.
#
# Variables
@ -538,6 +575,29 @@ foreach(_target_type,
"*",
TESTONLY_AND_VISIBILITY + [ "no_default_deps" ])
forward_variables_from(invoker, TESTONLY_AND_VISIBILITY)
if (!defined(inputs)) {
inputs = []
}
# Consumed by the unsafe-buffers plugin during compile.
#
# TODO(crbug.com/326584510): Reclient doesn't respect this variable, see
# rbe_bug_326584510_missing_inputs in //build/config/rbe.gni.
_uses_cflags = false
if (defined(sources)) {
foreach(f, sources) {
if (string_replace(f + ".END", ".cc.END", "") != f + ".END" ||
string_replace(f + ".END", ".c.END", "") != f + ".END" ||
string_replace(f + ".END", ".mm.END", "") != f + ".END" ||
string_replace(f + ".END", ".m.END", "") != f + ".END") {
_uses_cflags = true
}
}
}
if (_uses_cflags) {
inputs += [ "//build/config/unsafe_buffers_paths.txt" ]
}
if (!defined(deps)) {
deps = []
}

@ -2,6 +2,7 @@
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.
import("//build/buildflag_header.gni")
import("//build/config/rust.gni")
import("clang.gni")
@ -84,6 +85,28 @@ config("find_bad_constructs") {
}
}
# A plugin for incrementally applying the -Wunsafe-buffer-usage warning.
config("unsafe_buffers") {
if (clang_use_chrome_plugins) {
cflags = [
"-Xclang",
"-add-plugin",
"-Xclang",
"unsafe-buffers",
"-Xclang",
"-plugin-arg-unsafe-buffers",
"-Xclang",
rebase_path("//build/config/unsafe_buffers_paths.txt", root_build_dir),
]
}
}
buildflag_header("unsafe_buffers_buildflags") {
header = "unsafe_buffers_buildflags.h"
flags = [ "UNSAFE_BUFFERS_WARNING_ENABLED=$clang_use_chrome_plugins" ]
}
# Enables some extra Clang-specific warnings. Some third-party code won't
# compile with these so may want to remove this config.
config("extra_warnings") {

@ -1966,35 +1966,6 @@ config("prevent_unsafe_narrowing") {
}
}
# unsafe_buffer_warning -------------------------------------------------------
# Paths of third-party headers that violate Wunsafe-buffer-usage, but which we
# have been unable to fix yet. We use this list to be able to make progress and
# enable the warning on code that we do control/own.
#
# WARNING: This will disable all warnings in the files. ONLY USE THIS for
# third-party code which we do not control/own. Fix the warnings instead in
# our own code.
if (is_clang) {
unsafe_buffer_warning_header_allowlist =
[ "third_party/googletest/src/googletest/include/gtest" ]
}
# Enables warnings on pointer arithmetic/indexing or calls to functions
# annotated with `UNSAFE_BUFFER_USAGE`.
config("unsafe_buffer_warning") {
if (is_clang) {
cflags = [ "-Wunsafe-buffer-usage" ]
foreach(h, unsafe_buffer_warning_header_allowlist) {
if (is_win) {
cflags += [ "/clang:--system-header-prefix=$h" ]
} else {
cflags += [ "--system-header-prefix=$h" ]
}
}
}
}
# chromium_code ---------------------------------------------------------------
#
# Toggles between higher and lower warnings for code that is (or isn't)

@ -0,0 +1,22 @@
# Copyright 2024 The Chromium Project. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.
# The set of path prefixes that should be checked for unsafe buffer usage (see
# -Wunsafe-buffer-usage in Clang).
#
# ***
# Paths should be written as relative to the root of the source tree with
# unix-style path separators. Directory prefixes should end with `/`, such
# as `base/`.
# ***
#
# Files in this set are known to not use pointer arithmetic/subscripting, and
# make use of constructs like base::span or containers like std::vector instead.
#
# See `docs/unsafe_buffers.md`.
base/containers/buffer_iterator_nocompile.nc
base/containers/span.h
base/unsafe_buffers_unittest.cc
base/unsafe_buffers_nocompile.nc

@ -171,7 +171,7 @@ template("single_apple_toolchain") {
}
# C/C++ (clang) rewrapper prefix to use when use_remoteexec is true.
compiler_prefix = "${rbe_bin_dir}/rewrapper -cfg=${toolchain_rbe_cc_cfg_file} -exec_root=${rbe_exec_root} "
compiler_prefix = "${rbe_bin_dir}/rewrapper -cfg=${toolchain_rbe_cc_cfg_file}${rbe_bug_326584510_missing_inputs} -exec_root=${rbe_exec_root} "
} else if (toolchain_uses_goma) {
assert(toolchain_cc_wrapper == "",
"Goma and cc_wrapper can't be used together.")

@ -214,7 +214,7 @@ template("single_gcc_toolchain") {
}
# C/C++ (clang) rewrapper prefix to use when use_remoteexec is true.
compiler_prefix = "${rbe_bin_dir}/rewrapper -cfg=${toolchain_rbe_cc_cfg_file} -exec_root=${rbe_exec_root} "
compiler_prefix = "${rbe_bin_dir}/rewrapper -cfg=${toolchain_rbe_cc_cfg_file}${rbe_bug_326584510_missing_inputs} -exec_root=${rbe_exec_root} "
} else if (toolchain_uses_goma &&
(!defined(invoker.needs_gomacc_path_arg) ||
!invoker.needs_gomacc_path_arg)) {

@ -1,3 +1,7 @@
# 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.
# Defines the configuration of Remote Build Execution (RBE).
declare_args() {
@ -73,3 +77,13 @@ if (is_mac || is_ios) {
use_remoteexec_links = false
}
}
# TODO(crbug.com/326584510): Reclient does not upload `inputs` from C/C++
# targets. This file is added to `inputs` for all C targets in
# //build/config/BUILDCONFIG.gn. We work around the bug in Reclient by
# specifying the file here.
#
# This is a comma-delimited list of paths relative to the source tree root. The
# leading space is important, if the string is non-empty. :)
rbe_bug_326584510_missing_inputs =
" -inputs=build/config/unsafe_buffers_paths.txt"

@ -81,7 +81,7 @@ template("msvc_toolchain") {
if (toolchain_uses_remoteexec) {
if (toolchain_is_clang) {
cl_prefix = "${rbe_bin_dir}/rewrapper -cfg=${rbe_cc_cfg_file} -exec_root=${rbe_exec_root} -labels=type=compile,compiler=clang-cl,lang=cpp "
cl_prefix = "${rbe_bin_dir}/rewrapper -cfg=${rbe_cc_cfg_file}${rbe_bug_326584510_missing_inputs} -exec_root=${rbe_exec_root} -labels=type=compile,compiler=clang-cl,lang=cpp "
} else {
cl_prefix = ""
}

39
docs/unsafe_buffers.md Normal file

@ -0,0 +1,39 @@
# Preventing OOB through Unsafe Buffers errors (aka Spanification)
Out-of-bounds (OOB) security bugs commonly happen through pointers
which have no bounds checks associated with them. We prevent such
bugs by always using containers.
Most pointers are unowned references into an array (or vector)
and the most appropriate replacement for the pointer is
base::span.
When a file or directory is known to pass compilation with
`-Wunsafe-buffer-usage`,it should be added to the
[`//build/config/unsafe_buffers_paths.txt`](../build/config/unsafe_buffers_paths.txt)
file to enable compiler errors if unsafe pointer usage is added to
the file later.
# Functions with array pointer parameters
Functions that receive a pointer into an array may read
or write out of bounds of the pointer if given a pointer that
is incorrectly sized. Such functions should be marked with the
UNSAFE_BUFFER_USAGE attribute macro.
The same is true for functions that accept an iterator instead
of a range type. Some examples of each are memcpy() and
std::copy().
Calling such functions is unsafe and should generally be avoided.
Instead, replace such functions with an API built on base::span
or other range types which prevents any chance of OOB memory
access. For instance, replace `memcpy()`, `std::copy()` and
`std::ranges::copy()` with `base::span::copy_from()`. And
replace `memset()` with `std::ranges::fill()`.
# Writing unsafe data structures with pointers
TODO: Write about `UNSAFE_BUFFERS()` for rare exceptions where
the correctness of pointer bounds can be fully explained and
encapsulated, such as within a data structure.