Fix UNSAFE_TODO for wallet #26253
Fix UNSAFE_TODO for wallet #26253
Conversation
github-actions
bot
added
feature/web3/wallet
feature/web3/wallet/core
puLL-Merge
labels
| if (byte <= 0xf) { | ||
| std::string one_char_byte; | ||
| base::AppendHexEncodedByte(byte, one_char_byte, false); | ||
| result += one_char_byte[1]; |
There was a problem hiding this comment.
This just adds a single char, StrAppend is for appending many strings so doesn't fit here
supermassive
force-pushed
the
fix_unsafe_todo
branch
4 times, most recently
from
b2b95b6
to
4162d1e
Compare
| return std::nullopt; | ||
| } | ||
|
|
||
| // Parse first hop address. | ||
| path.push_back("0x" + HexEncodeLower(data.data(), 20)); | ||
| offset += 20; | ||
| path.push_back("0x" + HexEncodeLower(*reader.Read(20u))); |
| @@ -51,30 +45,26 @@ TEST(PermissionUtilsUnitTest, GetConcatOriginFromWalletAddresses) { | |||
| }}; | |||
|
|
|||
| url::Origin origin = url::Origin::Create(GURL("https://test.com")); | |||
| for (size_t i = 0; i < sizeof(cases) / sizeof(cases[0]); ++i) { | |||
| for (auto& test_case : cases) { | |||
| kBraveWalletPageGeneratedSize)), | ||
| IDR_WALLET_PAGE_HTML); | ||
| webui::SetupWebUIDataSource(source, | ||
| base::make_span(kBraveWalletPageGenerated), |
There was a problem hiding this comment.
This and all the other cases should be just base::span, and rely on CTAD. I say should because there could be a rare case where you still may need base::make_span until this CL lands through 132, but I think it would be good if we replaced base::make_span with base:span wherever it is possible to.
| @@ -71,7 +73,7 @@ class DiscoverAccountTaskBase { | |||
| uint32_t active_requests_ = 0; | |||
| // Indexed by 0 and 1 for receive and change addresses discovery states | |||
| // respectively. | |||
| State states_[2]; | |||
| std::array<State, 2> states_; | |||
There was a problem hiding this comment.
This works, but IMO it hides the original problem here: that we are using some arbitrary index to access two static states. I've went looking around and it seems the issue starts here:
struct BitcoinKeyId {
uint32 change;
uint32 index;
};
So we are storing this index as a number, when in fact there is only two options, namely, kBitcoinReceiveIndex and kBitcoinChangeIndex.
I think we should consider changing this into two fields, and actually turning BitcoinKeyId.index into BitcoinKeyId.type, the type being an enum. What do you think?
There was a problem hiding this comment.
That's a good point and I believe there is a TODO somewhere to switch to enum or a bool there. But that would be too much for this PR.
|
Would it be possible to break apart this PR into smaller separate PRs that are more manageable to review? These are more than mechanical changes, and I feel we could be missing things due to the sheer size of it. |
supermassive
force-pushed
the
fix_unsafe_todo
branch
from
4162d1e
to
3c3444a
Compare
|
[puLL-Merge] - brave/brave-core@26253 DescriptionThis PR makes significant changes to improve memory safety and modernize the codebase. It primarily replaces unsafe buffer handling with safer constructs, updates function signatures to use more appropriate types, and removes some compiler-specific workarounds. ChangesChanges
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
I understand the pain of reviewing this PR and it would be worth making these changes incremental from start. But I'm afraid that would consume too much time redoing it now. |
| @@ -3,12 +3,6 @@ | |||
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | |||
| * You can obtain one at https://mozilla.org/MPL/2.0/. */ | |||
|
|
|||
| #ifdef UNSAFE_BUFFERS_BUILD | |||
There was a problem hiding this comment.
nit: did we not have anything to fix in this one?
There was a problem hiding this comment.
Not sure what happened. Maybe corresponding included header triggered warnings
| return RLPTestStringToValue(s, &left_over); | ||
| base::Value RLPTestStringToValue(std::string s) { | ||
| base::ReplaceChars(s, "'", "\"", &s); | ||
| return base::test::ParseJson(s); |
I think we need to be a bit sensitive to reviewers time and bandwidth in digesting a problem. I don't think I can reasonably review this PR the state it is. |
|
Released in v1.73.78 |
| @@ -42,6 +38,13 @@ | |||
| using crypto::Encryptor; | |||
| using crypto::SymmetricKey; | |||
|
|
|||
| namespace base { | |||
There was a problem hiding this comment.
This is definitely not something we should be doing. You're effectively creating a layering violation with a kind of backdoor chromium src override for base.
There was a problem hiding this comment.
I basically wanted instances of this
template <size_t N>
struct SecureByteArray : public std::array<uint8_t, N> {
~SecureByteArray() { crypto::internal::SecureZeroBuffer(base::span(*this)); }
};
to be implicitly convertible to fixed size spans.
Will think how to achieve this in a different but succinct way.
This reverts commit e840557.
This reverts commit e840557. The original PR introduces a specialisation to `base::internal` that is not allowed, and would be Undefined Behaviour with `std::span`. This has broken `cr132`.
| secp256k1_pubkey public_key; | ||
| if (!secp256k1_ec_pubkey_create(GetSecp256k1Ctx(), &public_key, | ||
| private_key.data())) { | ||
| LOG(ERROR) << "secp256k1_ec_pubkey_create failed"; |
There was a problem hiding this comment.
Most logging should be removed before merge and at the very least this should be VLOG instead of error logs. Also using a log in place of returning actual error information using base::expected is an anti-pattern.
There was a problem hiding this comment.
I agree we don't need this logging. This is just some moved code which I didn't want to change much
Resolves brave/brave-browser#41664
Fixing
#pragma allow_unsafe_buffersandUNSAFE_TODOin wallet. Mostly this comes with using base::span (with compile-time size preferably) and adjusting dependent code.Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan: