Fix unsafe buffer usage in literal_buffer.h [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
// To avoid Integer OverflowBlink Style Guide: Prefer blink:: types over STL and base types. Use 'blink::wtf_size_t' instead of 'size_t' for 'count' and the 'base::CheckedNumeric' template argument to match the rest of the file.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
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. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
// To avoid Integer OverflowBlink Style Guide: Prefer blink:: types over STL and base types. Use 'blink::wtf_size_t' instead of 'size_t' for 'count' and the 'base::CheckedNumeric' template argument to match the rest of the file.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Will create a new bug to address this since this is not fixing memory safety.
Related details
- Arthur Sonzogni
- Kouhei Ueno
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. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
// To avoid Integer OverflowAditi PageBlink Style Guide: Prefer blink:: types over STL and base types. Use 'blink::wtf_size_t' instead of 'size_t' for 'count' and the 'base::CheckedNumeric' template argument to match the rest of the file.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Will create a new bug to address this since this is not fixing memory safety.
Done
Related details
- Arthur Sonzogni
- Kouhei Ueno
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno added 1 comment![]( "Open in Gerrit")
Mason: I'd like to rely on you to make the call. I worry the performance implication of the change in the hot code path. Should we request measurements?
Related details
- Aditi Page
- Arthur Sonzogni
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Mason: I'd like to rely on you to make the call. I worry the performance implication of the change in the hot code path. Should we request measurements?
Yeah, I'm definitely concerned about the same thing. This adds a ton of `CHECK` calls that likely slow down the parser significantly.
I'll try to kick some off.
Related details
- Aditi Page
- Arthur Sonzogni
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. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Mason FreedMason: I'd like to rely on you to make the call. I worry the performance implication of the change in the hot code path. Should we request measurements?
Yeah, I'm definitely concerned about the same thing. This adds a ton of `CHECK` calls that likely slow down the parser significantly.
I'll try to kick some off.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m4-mini-perf/speedometer3 failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12e6bfdf310000
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 1 comment![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | -1 |
1 comment
Hi Aditi,
Looking at the code, it doesn't compile for multiple reasons, including `-WUnsafe-buffer-usage`. In the future, could you please:
- Compile the code locally.
- Press the Commit-queue: +1 button and wait for the commit queue results before requesting a code review.
About this patch, I don't think it would be trivial to use AI to fix the UNSAFE_TODO here. Mostly likely, they will be replaced by `UNSAFE_BUFFERS` and a comment explaining why this is safe.
BTW, this isn't the original [AI patch](https://chromium-review.googlesource.com/c/chromium/src/+/7090591) that was proposed for the rotation.
Related details
- Aditi Page
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. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
Mason FreedMason: I'd like to rely on you to make the call. I worry the performance implication of the change in the hot code path. Should we request measurements?
Mason FreedYeah, I'm definitely concerned about the same thing. This adds a ton of `CHECK` calls that likely slow down the parser significantly.
I'll try to kick some off.
https://pinpoint-dot-chromeperf.appspot.com/job/12e6bfdf310000
- Kouhei Ueno
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
2 comments
This looks good to me! Thanks for the detailed (and correct-looking) new SAFETY comments.
It's a bit unfortunate that Arthur has marked this `-1` and then gone OOO. I'm afraid that'll make it unable to land without him removing his -1. But you have my LGTM for whenever that happens.
this->Grow(new_size);nit: you don't need `this->`.
Related details
- Aditi Page
- Kouhei Ueno
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. |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
this->Grow(new_size);nit: you don't need `this->`.
- Kouhei Ueno
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kouhei Ueno (Gerrit)
Kouhei Ueno voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kouhei Ueno
| Code-Review | +1 |
1 comment
- Aditi Page
- Nate Chapin
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
This looks good to me! Thanks for the detailed (and correct-looking) new SAFETY comments.
It's a bit unfortunate that Arthur has marked this `-1` and then gone OOO. I'm afraid that'll make it unable to land without him removing his -1. But you have my LGTM for whenever that happens.
Thank you for the review Mason and giving me more context on the performance aspect of this code change.
Related details
- Nate Chapin
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
LGTM to unblock. Do we have the pinpoint run?
I'll trigger it. Thanks!
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 3 comments![]( "Open in Gerrit")
Fix unsafe buffer usage in literal_buffer.h
Replaced raw pointer arithmetic in LiteralBufferBase with safer C++
constructs to prevent buffer overflows and other memory errors. This
includes using base::CheckedContiguousIterator for safe iteration,
adding bounds checks to pointer operations, and using
base::CheckedNumeric to prevent integer overflows.
Initial patchet generated by headless gemini-cli using:
//agents/prompts/projects/spanification/run.pyThe patch description needs to be updated to match to patch content.
void AppendLiteralImpl(const LiteralBufferBase<OtherT, kOtherSize>& val) {
static_assert(sizeof(T) >= sizeof(OtherT),
"T is not big enough to contain OtherT");
blink::wtf_size_t count = val.size();
blink::wtf_size_t new_size = size() + count;
if (capacity() < new_size) {
this->Grow(new_size);
}
// SAFETY: The `Grow()` call above ensures that there is enough capacity to
// append `count` characters.
UNSAFE_BUFFERS(std::copy_n(val.data(), count, end_));
UNSAFE_BUFFERS(end_ += count);
}A potential new integer overflow vulnerability here:
Switching from `size_t` to `wtf_size_t` typically reduced the size from 64bit to 32bit. Now `size() + count` could typically overflow.
This was `operator+(uint32_t, uint64_t)->uint64_t` and now it is `operator+(uint32_t, uint32_t)->uint32_t`.
Then:
```
// SAFETY: The `Grow()` call above ensures that there is enough capacity to
// append `count` characters.
UNSAFE_BUFFERS(std::copy_n(val.data(), count, end_));
```
become incorrect, because we might not have called "grow".
----
@mas...@chromium.org might know additional reasons why this couldn't fail?
If not we can:
- Use `base::CheckedNumeric::ValueOrDie()`, if performance allows it.
- Abandon and keep UNSAFE_TODO() here. We keep the problem open for future folks.
this->Grow(new_size);Aditi Pagenit: you don't need `this->`.
Done
- Aditi Page
- Nate Chapin
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+0![]( "Open in Gerrit")
| Code-Review | +0 |
Related details
- Aditi Page
- Nate Chapin
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m4-mini-perf/speedometer3 failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/138e8638710000
Mason Freed (Gerrit)
Mason Freed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
3 comments
Would be good to run another pinpoint, but this LGTM. I'll mark it.
void AppendLiteralImpl(const LiteralBufferBase<OtherT, kOtherSize>& val) {
static_assert(sizeof(T) >= sizeof(OtherT),
"T is not big enough to contain OtherT");
blink::wtf_size_t count = val.size();
blink::wtf_size_t new_size = size() + count;
if (capacity() < new_size) {
this->Grow(new_size);
}
// SAFETY: The `Grow()` call above ensures that there is enough capacity to
// append `count` characters.
UNSAFE_BUFFERS(std::copy_n(val.data(), count, end_));
UNSAFE_BUFFERS(end_ += count);
}A potential new integer overflow vulnerability here:
Switching from `size_t` to `wtf_size_t` typically reduced the size from 64bit to 32bit. Now `size() + count` could typically overflow.
This was `operator+(uint32_t, uint64_t)->uint64_t` and now it is `operator+(uint32_t, uint32_t)->uint32_t`.
Then:
```
// SAFETY: The `Grow()` call above ensures that there is enough capacity to
// append `count` characters.
UNSAFE_BUFFERS(std::copy_n(val.data(), count, end_));
```
become incorrect, because we might not have called "grow".----
@mas...@chromium.org might know additional reasons why this couldn't fail?
If not we can:
- Use `base::CheckedNumeric::ValueOrDie()`, if performance allows it.
- Abandon and keep UNSAFE_TODO() here. We keep the problem open for future folks.
Looks like the first one was chosen, which (perf-allowing) looks ok to me.
Aditi Pagenit: you don't need `this->`.
Arthur SonzogniDone
(Reopened, because this is not done)
- Aditi Page
- Arthur Sonzogni
- Kouhei Ueno
- Nate Chapin
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. |
Aditi Page (Gerrit)
Aditi Page added 3 comments![]( "Open in Gerrit")
Fix unsafe buffer usage in literal_buffer.h
Replaced raw pointer arithmetic in LiteralBufferBase with safer C++
constructs to prevent buffer overflows and other memory errors. This
includes using base::CheckedContiguousIterator for safe iteration,
adding bounds checks to pointer operations, and using
base::CheckedNumeric to prevent integer overflows.
Initial patchet generated by headless gemini-cli using:
//agents/prompts/projects/spanification/run.pyThe patch description needs to be updated to match to patch content.
Acknowledged
void AppendLiteralImpl(const LiteralBufferBase<OtherT, kOtherSize>& val) {
static_assert(sizeof(T) >= sizeof(OtherT),
"T is not big enough to contain OtherT");
blink::wtf_size_t count = val.size();
blink::wtf_size_t new_size = size() + count;
if (capacity() < new_size) {
this->Grow(new_size);
}
// SAFETY: The `Grow()` call above ensures that there is enough capacity to
// append `count` characters.
UNSAFE_BUFFERS(std::copy_n(val.data(), count, end_));
UNSAFE_BUFFERS(end_ += count);
}A potential new integer overflow vulnerability here:
Switching from `size_t` to `wtf_size_t` typically reduced the size from 64bit to 32bit. Now `size() + count` could typically overflow.
This was `operator+(uint32_t, uint64_t)->uint64_t` and now it is `operator+(uint32_t, uint32_t)->uint32_t`.
Then:
```
// SAFETY: The `Grow()` call above ensures that there is enough capacity to
// append `count` characters.
UNSAFE_BUFFERS(std::copy_n(val.data(), count, end_));
```
become incorrect, because we might not have called "grow".----
@mas...@chromium.org might know additional reasons why this couldn't fail?
If not we can:
- Use `base::CheckedNumeric::ValueOrDie()`, if performance allows it.
- Abandon and keep UNSAFE_TODO() here. We keep the problem open for future folks.
Acknowledged
this->Grow(new_size);Aditi Pagenit: you don't need `this->`.
Arthur SonzogniDone
(Reopened, because this is not done)
- Arthur Sonzogni
- Kouhei Ueno
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
Would be good to run another pinpoint, but this LGTM. I'll mark it.
Yes, thanks for looking at this!
Related details
- Arthur Sonzogni
- Kouhei Ueno
- Nate Chapin
Code-Coverage
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m4-mini-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/134bd2b8710000
Related details
- Aditi Page
- Arthur Sonzogni
- Kouhei Ueno
- Nate Chapin
Code-Coverage
Code-Owners
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
Aditi PageWould be good to run another pinpoint, but this LGTM. I'll mark it.
Yes, thanks for looking at this!
I've completed the pinpoint run comparing Patchset 7 against HEAD. From the run, we can conclude that memory safety improvements in this CL do not appear to regress the parser performance. Thanks again for the guidance on the performance verification!
Link to pinpoint run: https://pinpoint-dot-chromeperf.appspot.com/job/134bd2b8710000
Related details
Kouhei Ueno (Gerrit)
Kouhei Ueno voted Code-Review+1![]( "Open in Gerrit")
- Aditi Page
- Arthur Sonzogni
- Nate Chapin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nate Chapin (Gerrit)
Nate Chapin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Aditi Page
- Arthur Sonzogni
Arthur Sonzogni (Gerrit)
Arthur Sonzogni added 1 comment![]( "Open in Gerrit")
Fix unsafe buffer usage in literal_buffer.h
Replaced raw pointer arithmetic in LiteralBufferBase with safer C++
constructs to prevent buffer overflows and other memory errors. This
includes using base::CheckedContiguousIterator for safe iteration,
adding bounds checks to pointer operations, and using
base::CheckedNumeric to prevent integer overflows.
Initial patchet generated by headless gemini-cli using:
//agents/prompts/projects/spanification/run.py
Fixed: 455863178Could you please keep the commit message up to date?
---
Subject: Fix unsafe buffer usage literal_buffer.h
Replaced raw pointer arithmetic annotations in LiteralBufferBase with
safe C++ constructs or UNSAFE_BUFFERS macros.
Key changes include:
- using base::CheckedNumeric to prevent integer overflows during
buffer resizing and appending.
- replacing UNSAFE_TODO with UNSAFE_BUFFERS, accompanied by detailed
SAFETY comments explaining why bounds checks are satisfied.
- updating size/capacity types to blink::wtf_size_t for consistency.
Fixed: 455863178
Related details
- Aditi Page
- Arthur Sonzogni
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. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Aditi Page (Gerrit)
Aditi Page added 1 comment![]( "Open in Gerrit")
Fix unsafe buffer usage in literal_buffer.h
Replaced raw pointer arithmetic in LiteralBufferBase with safer C++
constructs to prevent buffer overflows and other memory errors. This
includes using base::CheckedContiguousIterator for safe iteration,
adding bounds checks to pointer operations, and using
base::CheckedNumeric to prevent integer overflows.
Initial patchet generated by headless gemini-cli using:
//agents/prompts/projects/spanification/run.py
Fixed: 455863178Could you please keep the commit message up to date?
---
Subject: Fix unsafe buffer usage literal_buffer.h
Replaced raw pointer arithmetic annotations in LiteralBufferBase with
safe C++ constructs or UNSAFE_BUFFERS macros.Key changes include:
- using base::CheckedNumeric to prevent integer overflows during
buffer resizing and appending.
- replacing UNSAFE_TODO with UNSAFE_BUFFERS, accompanied by detailed
SAFETY comments explaining why bounds checks are satisfied.
- updating size/capacity types to blink::wtf_size_t for consistency.
Fixed: 455863178
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditi Page (Gerrit)
Aditi Page voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Fix unsafe buffer usage in literal_buffer.h
Replaced raw pointer arithmetic annotations in LiteralBufferBase with
safe C++ constructs or UNSAFE_BUFFERS macros.
Key changes include:
- Using base::CheckedNumeric to prevent integer overflows during
buffer resizing and appending.
- Replacing UNSAFE_TODO with UNSAFE_BUFFERS, accompanied by detailed
SAFETY comments explaining why bounds checks are satisfied.
- Updating size/capacity types to blink::wtf_size_t for consistency.
Initial patchet generated by headless gemini-cli using:
//agents/prompts/projects/spanification/run.py
- M third_party/blink/renderer/core/html/parser/literal_buffer.h
Code-Review: +1 by Arthur Sonzogni, +1 by Nate Chapin, +1 by Mason Freed, +1 by Kouhei Ueno
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |