Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AtlasEngine: Fix a buffer overrun #17536

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jul 9, 2024

The strided memcpy between buffers failed to account for situations
where the destination stride is smaller than the source stride.
The solution is to only copy as many bytes as are in each row.

Validation Steps Performed

Even with AppVerifier the issue could not be reproduced.
Adding an assert(srcStride <= mapped.RowPitch), however, did trap
the bug when WARP is used while BackendD3D is force-enabled.

@lhecker lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) Area-AtlasEngine labels Jul 9, 2024
@DHowett
Copy link
Member

DHowett commented Jul 9, 2024

If I understand correctly, the source stride is perfectly tuned to be the right size. The destination stride is perfectly tuned for the hardware and the size. How is it possible for us to copy the right amount of data from the source, when the destination stride is shorter, and not corrupt the background bitmap?

@lhecker
Copy link
Member Author

lhecker commented Jul 9, 2024

The source stride and destination stride are both >= the bitmap width, but that doesn't mean that the destination stride is >= than the source stride. Copying only the bitmap width is the right thing to do and won't corrupt the bitmap.

@lhecker lhecker added this pull request to the merge queue Jul 9, 2024
Merged via the queue into main with commit ae8c868 Jul 9, 2024
20 checks passed
@lhecker lhecker deleted the dev/lhecker/atlas-engine-oob-fix branch July 9, 2024 22:42
DHowett pushed a commit that referenced this pull request Jul 9, 2024
The strided `memcpy` between buffers failed to account for situations
where the destination stride is smaller than the source stride.
The solution is to only copy as many bytes as are in each row.

## Validation Steps Performed
Even with AppVerifier the issue could not be reproduced.
Adding an `assert(srcStride <= mapped.RowPitch)`, however, did trap
the bug when WARP is used while BackendD3D is force-enabled.

(cherry picked from commit ae8c868)
Service-Card-Id: 92972719
Service-Version: 1.20
@zadjii-msft zadjii-msft linked an issue Jul 10, 2024 that may be closed by this pull request
DHowett pushed a commit that referenced this pull request Aug 13, 2024
The strided `memcpy` between buffers failed to account for situations
where the destination stride is smaller than the source stride.
The solution is to only copy as many bytes as are in each row.

## Validation Steps Performed
Even with AppVerifier the issue could not be reproduced.
Adding an `assert(srcStride <= mapped.RowPitch)`, however, did trap
the bug when WARP is used while BackendD3D is force-enabled.

(cherry picked from commit ae8c868)
Service-Card-Id: 92972720
Service-Version: 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-AtlasEngine Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal.
Projects
Status: Cherry Picked
Development

Successfully merging this pull request may close these issues.

Resizing Windows Terminal causes the app to close
3 participants