Add per-source opt-in support for -Wunsafe-buffer-usage#3217
Add per-source opt-in support for -Wunsafe-buffer-usage#3217metsw24-max wants to merge 4 commits into
-Wunsafe-buffer-usage#3217Conversation
| // Copyright 2023 Google LLC | ||
| // SPDX-License-Identifier: BSD-2-Clause | ||
|
|
||
| #include <array> |
There was a problem hiding this comment.
metsw24-max: Thank you for the pull request. I'd like to learn more about Clang's -Wunsafe-buffer-usage flag. Is it for C++ code only?
Could you please move this file to a separate pull request? The changes to this file could stand on their own (replacing C arrays with std::array).
There was a problem hiding this comment.
Hi @wantehchang, thanks for the review!
Regarding -Wunsafe-buffer-usage: yes, it is effectively a C++-only warning. Clang introduced it as part of the "C++ Safe Buffers" programming model. It flags unsafe pointer arithmetic and raw-pointer indexing, and its suggested fixes point at C++ types like std::span and std::array (and hardened libc++). It can technically be enabled on C translation units, but the diagnostics and fix-its don't really apply there, so we only opt in C++ sources.
Happy to split out the tests/gtest/avifrgbtest.cc changes into a separate PR
you are right that the std::array cleanup stands on its own. I'll drop it from this PR and open a follow-up.
| const std::string command_name(argv[1]); | ||
| // argv is a raw pointer with no associated bound, so -Wunsafe-buffer-usage | ||
| // cannot prove these accesses safe. They are bounded by the argc checks | ||
| // above and by the host's contract with main(): argv[0..argc-1] are valid. |
There was a problem hiding this comment.
Nit: argv[argc] is also valid and is a null pointer. But most code I have seen only uses argv[0..argc-1].
| if (command->name() == command_name) { | ||
| try { | ||
| avifResult result = command->ParseArgs(argc - 1, argv + 1); | ||
| avifResult result = command->ParseArgs(argc - 1, command_argv); |
There was a problem hiding this comment.
Nit: The original code has a nice symmetry between argc - 1 and argv + 1:
avifResult result = command->ParseArgs(argc - 1, argv + 1);
This symmetry is lost in the new code. We could address this by saving argc - 1 in a new variable command_argc in conjunction with command_argv.
| #if __has_warning("-Wunsafe-buffer-usage") | ||
| #pragma clang unsafe_buffer_usage end | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
cc: @maryla-uc
Maryla: You are the author of this file. Although I expressed my opinion below, I defer the decision to you.
metsw24-max: I find the new code less clear than the original code, so the trade-off does not seem worthwhile for this main() function.
An experienced programmer can easily see this main() function uses the argv array safely by searching for "argv" and verifying argv is only used with an index less than argc.
Here you annotated three accesses to argv as unsafe. Is it possible to simply annotate the argv variable as unsafe?
Note: Clang's Safe C++ Buffers page recommends handling this kind of case using C++20's std:: span. But libavif follows Google's Foundational C++ Support table and needs to stick with C++17 until 2027-12-15.
| } | ||
|
|
||
| const std::string command_name(argv[1]); | ||
| // argv is a raw pointer with no associated bound, so -Wunsafe-buffer-usage |
There was a problem hiding this comment.
Nit: argv is associated with the bound argc. It's just that -Wunsafe-buffer-usage doesn't understand this association. I suggest changing this comment to "-Wunsafe-buffer-usage doesn't know the raw pointer argv is associated with the bound argc, so it cannot prove these accesses safe."
| const std::string command_name(argv[1]); | ||
| // argv is a raw pointer with no associated bound, so -Wunsafe-buffer-usage | ||
| // cannot prove these accesses safe. They are bounded by the argc checks | ||
| // above and by the host's contract with main(): argv[0..argc-1] are valid. |
There was a problem hiding this comment.
Nit: Remove "above" from "the argc checks above", because the argc >= 3 check is below.
|
Thanks @wantehchang for the detailed feedback. I have pushed an update that:
The |
2 participants
Adds infrastructure for selectively enabling Clang’s
-Wunsafe-buffer-usagediagnostic on audited C++ source files.Changes:
Detect compiler support for
-Wunsafe-buffer-usageAdd
avif_enable_safe_buffers_warning()helper for per-source opt-inEnable the warning for:
src/compliance.ccapps/avifgainmaputil/*.ccConsolidate raw
argvaccess inavifgainmaputil.ccinto a single guarded region using:#ifdef __clang____has_warning("-Wunsafe-buffer-usage")#pragma clang unsafe_buffer_usage begin/end