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