Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: re-enable unsafe buffer checks #45770

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Feb 22, 2025

Description of Change

Re-enable the unsafe-buffer checks that were disabled by 1e9acec in the 134.0.6988.0 Chromium roll (#45334).

Fixes #45411.

Checklist

Maintainer Notes

I'd never poked around in Chromium's Clang plugins before, so in case I (or another maintainer) has to do it again: I tracked this down by patching tools/clang/plugins/UnsafeBuffersPlugin.cpp with these new log statements and then rebuilding clang by following these instructions. Then I used that build of Clang to rebuild Electron. To ensure the patched clang got used, I passed the --offline flag to reclient/rewrapper.

Warnings in net/cookies/parsed_cookie.cc:

../../net/cookies/parsed_cookie.cc:90:45: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
   90 |   for (; *it != end && **it != character; ++(*it)) {
      |                                             ^~~~~
../../net/cookies/parsed_cookie.cc:90:45: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:100:49: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  100 |   for (; *it != end && !CharIsA(**it, chars); ++(*it)) {
      |                                                 ^~~~~
../../net/cookies/parsed_cookie.cc:100:49: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:109:48: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  109 |   for (; *it != end && CharIsA(**it, chars); ++(*it)) {
      |                                                ^~~~~
../../net/cookies/parsed_cookie.cc:109:48: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:116:48: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  116 |   for (; *it != end && CharIsA(**it, chars); --(*it)) {
      |                                                ^~~~~
../../net/cookies/parsed_cookie.cc:116:48: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:308:13: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  308 |     end = s.begin() + term_pos;
      |           ~~^~~~~~~
../../net/cookies/parsed_cookie.cc:308:13: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:337:7: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  337 |     --(*it);                  // Go back before the token separator.
      |       ^~~~~
../../net/cookies/parsed_cookie.cc:337:7: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:341:7: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  341 |     ++(*it);
      |       ^~~~~
../../net/cookies/parsed_cookie.cc:341:7: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:371:7: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  371 |     --(*value_end);
      |       ^~~~~~~~~~~~
../../net/cookies/parsed_cookie.cc:371:7: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:375:7: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  375 |     ++(*value_end);
      |       ^~~~~~~~~~~~
../../net/cookies/parsed_cookie.cc:375:7: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:571:9: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  571 |       ++it;  // Skip past the '='.
      |         ^~
../../net/cookies/parsed_cookie.cc:571:9: note: See //docs/unsafe_buffers.md for help.
/home/charles/electron/gn/main/src/tools/clang/plugins/UnsafeBuffersPlugin.cpp:238 filename ../../net/cookies/parsed_cookie.cc
243 returning from cache: 1
../../net/cookies/parsed_cookie.cc:634:9: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
  634 |       ++it;
      |         ^~
../../net/cookies/parsed_cookie.cc:634:9: note: See //docs/unsafe_buffers.md for help.

Release Notes

Notes: none.

@ckerr ckerr added security 🔒 semver/patch backwards-compatible bug fixes upgrade-follow-up Stuff left to do after a Chromium upgrade. target/35-x-y PR should also be added to the "35-x-y" branch. labels Feb 22, 2025
@ckerr ckerr requested a review from a team as a code owner February 22, 2025 20:04
@ckerr ckerr force-pushed the chore/reenable-unsafe-buffer-checks branch from 739ee02 to 596b457 Compare February 22, 2025 20:50
@deepak1556
Copy link
Member

Thanks for re-enabling the warnings!

@ckerr can you help clarify why we have some directories in https://github.com/electron/electron/blob/main/electron_unsafe_buffers_paths.txt while they are no longer present in https://source.chromium.org/chromium/chromium/src/+/main:build/config/unsafe_buffers_paths.txt. For ex: net/ in this case https://chromium-review.googlesource.com/c/chromium/src/+/5563610, why did upstream consider the above file safe for unsafe buffer warnings ?

@ckerr
Copy link
Member Author

ckerr commented Feb 24, 2025

@deepak1556 two different answers:

  • From the perspective of our own txt file: IMO unsafe buffer warnings should only break our build if the warning in C++ code that we maintain, i.e. shell/. I left -net/ in our config so that warnings there won't break our build.

  • Clearly there is code in net/cookies/parsed_cookie.cc that generates unsafe buffer warnings. But since Chromium builds & releases keep happening, that doesn't seem to be a blocker for them. So maybe they don't build with -Werror, or maybe they don't do production builds with buffer warnings enabled? Something odd is going on there.

@ckerr ckerr force-pushed the chore/reenable-unsafe-buffer-checks branch from 596b457 to ca9734b Compare February 24, 2025 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security 🔒 semver/patch backwards-compatible bug fixes target/35-x-y PR should also be added to the "35-x-y" branch. upgrade-follow-up Stuff left to do after a Chromium upgrade.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Upgrade Follow-Up]: Re-enable clang_unsafe_buffers_paths
2 participants