Repository navigation
Fix HostBuffer.emplace overflow at block boundary - #186539
soulman-is-good wants to merge 1 commit into
Conversation
github-actions
Bot
added
engine
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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
bdero
left a comment
bdero
left a comment
There was a problem hiding this comment.
Thanks for the clear writeup and the platform-independent test sizes. The boundary fix is correct, and the strict > is the right call, since a write that exactly fills the block is valid.
While we're in this branch, there's a second pre-existing bug that your fix now exercises much more often. When _allocateEmplacement rolls over to a fresh block it returns the view with lengthInBytes set to blockLengthInBytes instead of bytes.lengthInBytes, so callers get a view spanning the whole block rather than just the payload. The old bounds check almost never took this branch cleanly (it crashed instead), so the wrong length stayed masked. Now that this is the normal rollover path, every rollover hands back an oversized view. The other two return paths already use bytes.lengthInBytes. Would you mind fixing that line and extending the new test to cover it? Specifics inline.
| // cursor lands inside `[blockLengthInBytes - bytes.lengthInBytes, | ||
| // blockLengthInBytes)` the check would pass, but the subsequent | ||
| // `DeviceBuffer.overwrite` would reject the out-of-bounds write. | ||
| if (_offsetCursor + padding + bytes.lengthInBytes > blockLengthInBytes) { |
There was a problem hiding this comment.
The return BufferView at the end of this branch should use bytes.lengthInBytes, not blockLengthInBytes. This branch writes bytes at offset 0 of the freshly allocated block, so the returned view should describe the payload, matching the oversized branch above and the normal path below. As written, every rollover returns a view spanning the entire block, which then gets bound as an oversized uniform or vertex range.
| // The new view must reference a freshly-allocated block, not the | ||
| // original one, and start at offset 0 of that block. | ||
| expect(view.buffer, isNot(equals(fillView.buffer))); | ||
| expect(view.offsetInBytes, 0); |
There was a problem hiding this comment.
Could you also assert the length here with expect(view.lengthInBytes, overflowingSize);? It pins the view to the payload size, and it currently fails because of the lengthInBytes bug I flagged in buffer.dart, so this one assertion locks in both fixes.
|
Greetings from stale PR triage! 👋 |
Piinks
added
the
waiting for response
|
This pull request is being closed because it has not been updated in the last 21 days after a request for more information. If you are still working on this, please feel free to reopen it or file a new pull request. Thanks for your contribution. |
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.