-
Notifications
You must be signed in to change notification settings - Fork 859
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
Use base::StringPrintf
in constexrp
contexts
#25643
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wallet-core++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think sec team was tagged for the network audit whitelist change but @SergeyZhukovsky should take a look at it
32d4f43
to
e61704e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
browser/net
Upstream has made this function constexpr, in order to force callers to change its call sites to use a constexpr string, in order to be able to validate at compile time the format string. There is an alternative `base::StringPrintfNonConstexpr`, but obviously this unsafe variant should be avoided whenever possible. This change goes over the different places where the `constexpr` constrained was being violated, and corrects each callsite. This has also revealed a buggy test in `SimulationResponseParserUnitTest`, where the formatting string wasn't correct. Chromium change: https://chromium.googlesource.com/chromium/src/+/8bf39fbefcd2963b3647c24f2b27dfb5c40875c4 commit 8bf39fbefcd2963b3647c24f2b27dfb5c40875c4 Author: Peter Kasting <pkasting@chromium.org> Date: Fri Sep 13 19:07:33 2024 +0000 Disallow StringPrintf() with non-constexpr format strings. Bug: 365705855
e61704e
to
133e626
Compare
[puLL-Merge] - brave/brave-core@25643 DescriptionThis PR makes several changes to improve code quality and consistency across multiple components of the Brave browser. The main changes involve replacing string formatting methods, removing unused code, and refactoring some functions for better readability and maintainability. ChangesChanges
Overall, these changes aim to improve code readability, reduce string allocations, and make better use of C++ language features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++Rewards
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
Upstream has made this function constexpr, in order to force callers to change its call sites to use a constexpr string, in order to be able to validate at compile time the format string. There is an alternative
base::StringPrintfNonConstexpr
, but obviously this unsafe variant should be avoided whenever possible.This change goes over the different places where the
constexpr
constrained was being violated, and corrects each callsite. This has also revealed a buggy test inSimulationResponseParserUnitTest
, where the formatting string wasn't correct.Chromium change:
https://chromium.googlesource.com/chromium/src/+/8bf39fbefcd2963b3647c24f2b27dfb5c40875c4
Resolves brave/brave-browser#41137
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan: