Fix HostBuffer.emplace overflow at block boundary#186539
Fix HostBuffer.emplace overflow at block boundary#186539soulman-is-good wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the HostBuffer allocation logic to include payload size in bounds checks, preventing out-of-bounds writes when the cursor is near the end of a block. It also introduces a regression test to verify this behavior. Review feedback identified a copy-paste error in the test assertions where the view length was incorrectly compared to the total block size instead of the payload size.
| // original one, and start at offset 0 of that block. | ||
| expect(view.buffer, isNot(equals(fillView.buffer))); | ||
| expect(view.offsetInBytes, 0); | ||
| expect(view.lengthInBytes, blockLength); |
There was a problem hiding this comment.
The expectation for view.lengthInBytes appears to be a copy-paste error. It should be checked against overflowingSize (the size of the data emplaced), which is consistent with how fillView.lengthInBytes was checked against fillSize on line 281. Checking against blockLength would imply the view covers the entire block, which is not the behavior of emplace.
| expect(view.lengthInBytes, blockLength); | |
| expect(view.lengthInBytes, overflowingSize); |
There was a problem hiding this comment.
I removed unrelated assertion so the test only checks related things
`HostBuffer._allocateEmplacement` decided whether to allocate a new
block based on the post-padding cursor only, ignoring the size of the
data being emplaced. When the cursor landed inside the range
`[blockLengthInBytes - bytes.lengthInBytes, blockLengthInBytes)`, the
check passed but the subsequent `DeviceBuffer.overwrite` rejected the
write because it would extend past the block end, throwing:
Failed to write range (offset=..., length=...) to HostBuffer-managed
DeviceBuffer (frame=0, buffer=0, offset=...).
This was reproducible in flame_3d 3D apps once per-frame uniform traffic
neared 1 MB at certain camera angles.
Include the upcoming write size in the boundary check:
if (_offsetCursor + padding + bytes.lengthInBytes > blockLengthInBytes) {
The strict `>` (rather than `>=`) is intentional: a write that exactly
fills the remaining space is valid and shouldn't waste a new block.
Adds a regression test that exercises the boundary by configuring a
small `blockLengthInBytes` and emplacing a payload that would overflow
the current block under the previous check.
Fixes flutter#186526
9b4b631 to
65b7a68
Compare
HostBuffer._allocateEmplacementdecided whether to allocate a new block based on the post-padding cursor only, ignoring the size of the data being emplaced. When the cursor landed inside the range[blockLengthInBytes - bytes.lengthInBytes, blockLengthInBytes), the check passed but the subsequentDeviceBuffer.overwriterejected the write because it would extend past the block end, throwing:Failed to write range (offset=..., length=...) to HostBuffer-managed
DeviceBuffer (frame=0, buffer=0, offset=...).
This was reproducible in flame_3d 3D apps once per-frame uniform traffic neared 1 MB at certain camera angles.
Include the upcoming write size in the boundary check:
The strict
>(rather than>=) is intentional: a write that exactly fills the remaining space is valid and shouldn't waste a new block.Adds a regression test that exercises the boundary by configuring a small
blockLengthInBytesand emplacing a payload that would overflow the current block under the previous check.Fixes #186526
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. You must list at least one issue. An issue is not required if the PR fixes something trivial like a typo.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.