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 bounds checking when writing to the temp index buffer when expanding primitives #17387

Closed
wants to merge 2 commits into from

Conversation

hrydgard
Copy link
Owner

@hrydgard hrydgard commented May 2, 2023

Not sure whether it make the most sense to:

  • Add these checks
  • Increase the size of the temp index buffer until there's no risk for overrun
  • Put caps on the size of spline draws, which I think is the cause of the crash found in 1.15 Android mystery crash thread #17364

Opinions? At least this one feels the safest...

Copy link
Collaborator

@unknownbrackets unknownbrackets left a comment

Choose a reason for hiding this comment

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

Seems reasonable, I think it might be a bit cleaner to use a field on SoftwareTransformResult for the index buffer it wrote to, though.

-[Unknown]

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.

Maybe it'd be cleaner for result.indsBuffer or something to exist, and be the resulting index pointer. We already use result.drawBuffer, so it'd make the code more symmetrical and probably easier to understand.

-[Unknown]

bool SoftwareTransform::ExpandPoints(int vertexCount, int &maxIndex, u16 *inds, int &indsOffset, int indexBufferSize, const TransformedVertex *transformed, TransformedVertex *transformedExpanded, int &numTrans, bool throughmode) {
// Before we start, do a sanity check - does the output fit?
if (vertexCount * 6 > indexBufferSize - indsOffset) {
// Won't fit, kill the draw.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably log, if not report this situation. It could mean it's running garbage (most likely?) but might actually be some kind of effect.

-[Unknown]

@unknownbrackets
Copy link
Collaborator

I feel like we found in FF4 or some game that surprisingly high resulting vertex counts from spline/bezier are valid, so I worry about trying to cap it incorrectly...

-[Unknown]

@hrydgard hrydgard modified the milestones: v1.15.1, v1.16.0 May 3, 2023
@hrydgard
Copy link
Owner Author

hrydgard commented May 5, 2023

Closing this, seems increasing the buffer size did the trick as I haven't seen any repeat of this yet. Though I do want to make things clearer and more "intentional" around these buffers at some point.

@hrydgard hrydgard closed this May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants