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

Add memory bounds-check when expanding points, rects and lines to triangles #18184

Merged
merged 2 commits into from
Sep 20, 2023

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented Sep 20, 2023

I keep seeing a Google Play crash report pointing to ExpandLines, it's not super common, but it should be readily fixable. My guess is that it happens when a game has already gone off-trails and is executing garbage data, since such huge line draws seem weird, but who knows.

I did this change a long time ago but never got it in for some reason. This does a little refactoring and then fixes it, I believe.

Not sure if I should push it into 1.16.3. Maybe...

@hrydgard hrydgard added the GE emulation Backend-independent GPU issues label Sep 20, 2023
@hrydgard hrydgard added this to the v1.16.3 milestone Sep 20, 2023
@hrydgard
Copy link
Owner Author

Since the changes only touch the software transform path, and are rather minimal, I wouldn't expect any performance loss.

Self-reviewed it again, decided I'll get it in.

@hrydgard hrydgard merged commit 65b995a into master Sep 20, 2023
18 checks passed
@hrydgard hrydgard deleted the expand-lines-mem-fix branch September 20, 2023 18:39
u16 *newInds = inds + vertexCount;
u16 *indsOut = newInds;
const u16 *indsIn = (const u16 *)(inds + indsOffset);
int newIndsOffset = indsOffset + vertexCount;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this not add enough space to not overlap? Not sure if maybe it's still safe. Before it was (u16 *)x + y aka x + y * 2 and now it's x + y. Also in other places here.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh duh, sloppy of me to miss. My test cases still worked somehow though..

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually hm, indsOffset is in 16-bit units here, since we just after add it to a u16*. But I think there might be some unclarity here, will check it all.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought one of these had been changed to a u8 because of the cast, I guess. My mistake.

-[Unknown]

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, something was indeed wrong and I had to revert, but maybe not here. Will redo this later.

hrydgard added a commit that referenced this pull request Sep 27, 2023
hrydgard added a commit that referenced this pull request Sep 27, 2023
Revert "Merge pull request #18184 from hrydgard/expand-lines-mem-fix"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GE emulation Backend-independent GPU issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants