PA: Zap infinite-quarantined memory regions with their class specific key. [chromium/src : main]
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 2 comments![]( "Open in Gerrit")
static constexpr uint32_t kTypeId = [] { \Takashi SakamotoYou'll want a more namespaced variable name I think. Something like kPartitionAllocSanitizedObjectTypeId?
Done
Related details
- Keishi Hattori
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keishi Hattori (Gerrit)
Keishi Hattori voted and added 2 comments![]( "Open in Gerrit")
Votes added by Keishi Hattori
| Code-Review | +1 |
2 comments
LGTM
// And everything else is just empyt padding.typo
Related details
- Stephen Nusko
- Takashi Sakamoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 2 comments![]( "Open in Gerrit")
// And everything else is just empyt padding.Takashi Sakamototypo
Done
Related details
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Sakamoto (Gerrit)
Takashi Sakamoto added 1 comment![]( "Open in Gerrit")
- Greg Thompson
- Stephen Nusko
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Greg Thompson (Gerrit)
Greg Thompson added 11 comments![]( "Open in Gerrit")
Hi. I stopped reviewing once I noticed that we're only using the first 32 bits of the MD5 hash digest. See my comment about this on line 249 of base/memory/advanced_memory_safety_checks_hash.h. I suggest two simpler alternatives there. What do you think about changing the algorithm?
PA: Zap infinite-quarantined memory regions with their class specific key.tiny nit: omit punctuation at the end of the summary line as per https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#uploading-a-change-for-review and https://chris.beams.io/git-commit#end
constexpr uint32_t kHash = \
base::internal::MD5Hash32Constexpr(__PRETTY_FUNCTION__); \
return kHash; \can this be simplified to:
```
return base::internal::MD5Hash32Constexpr(__PRETTY_FUNCTION__);
```
?
return internal::MD5CE::Hash32(string);nit: omit `internal::`
constexpr uint32_t MD5Hash32Constexpr(std::string_view string) {it's a bit confusing that this function doesn't return an MD5 hash digest. if we're only using the first 32 bits of the digest, why are we using MD5?
have you considered MurmurHash3 (example: https://source.chromium.org/chromium/chromium/src/+/main:components/safe_browsing/content/renderer/phishing_classifier/murmurhash3_util.cc) or FNV-1a (example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/include/perfetto/tracing/internal/fnv1a.h)?
constexpr uint32_t MD5Hash32Constexpr(std::string_view string) {could we use `base::span<const uint8_t>` rather than `std::string_view`? callers can use `base::byte_span_from_cstring()`, `base::as_bytes()`, or `base::as_byte_span()` depending on what they want to compute a hash digest over.
in either case, please #include either <string_view> or "base/containers/span.h" depending on which way you go.
return ((a & 0xff) << 24) | (((a >> 8) & 0xff) << 16) |we have `ExtractByte` above. could we use that here rather than sort of reimplementing it? i would hope that the compiler is smart enough to optimize down to more-or-less the same thing. or do you find otherwise?
const uint32_t m =https://google.github.io/styleguide/cppguide.html#General_Naming_Rules says we generally shouldn't abbreviate. `padded_length` would be a good name for this.
return (((n + 1 + 8) + 63) / 64) * 64;this could overflow. could we use [`CheckedNumeric`](https://chromium.googlesource.com/chromium/src/+/main/base/numerics/README.md#checkednumeric_in-checked_math_h) here?
struct MD5CE {the style guide says ["Do not use a class simply to group static members"](https://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_and_Global_Functions). a namespace (with `inline constexpr` for the constants) would suffice.
please add a unit test for this that at least validates the test vectors from RFC 1321 in appendix [A.5](https://datatracker.ietf.org/doc/html/rfc1321#appendix-A.5). the ones from [RFC 2202](https://datatracker.ietf.org/doc/html/rfc2202#section-2) would be great, too. and NIST has some test vectors [here](https://www.nist.gov/itl/ssd/software-quality-group/nsrl-test-data).
Related details
- Stephen Nusko
- Takashi Sakamoto
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |