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

Fix memory corruption and assert failures in convex decomposition #85631

Merged

Conversation

Malcolmnixon
Copy link
Contributor

@Malcolmnixon Malcolmnixon commented Dec 1, 2023

This PR fixes issue 85439 by changing the index pre-increment to post-increment so index 0 is not skipped and preventing a potential overrun past the end of the vertices vector.

Testing involved constructing a GLB file consisting of a single triangle:
image

Before this change the code allocates vertices and indices arrays of size 3 and writes the following:

Vertices (size=3) Indices (size=3)
0 = unused 1
1 = [-1,-1,-1] 2
2= [1,-1,-1] 3
3 = [0,1,-1] (BUFFER OVERRUN) unused

After this change the code writes the following:

Vertices (size=3) Indices (size=3)
0 = [-1,-1,-1] 0
1 = [1,-1,-1] 1
2= [0,1,-1] 2

The collision shape with this new data appears correct:
image

offset_triangle.zip

This PR fixes how triangular faces are decomposed into vertices and indices. The pre-increment resulted in the indices table skipping entry 0 and potentially overrunning the end of the vertices vector.
@fire fire requested a review from a team December 2, 2023 01:41
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Makes sense to me, would like the original author to weigh in if there was a reason for this. This is not something people tent to do by accident, most likely there was other code here before that makes this make sense..

@Malcolmnixon
Copy link
Contributor Author

Makes sense to me, would like the original author to weigh in if there was a reason for this. This is not something people tent to do by accident, most likely there was other code here before that makes this make sense..

It looks to have been introduced with 2ca94e5 and no code before it seems to implement the vertex-deduplication map trick. The older code uses duplicated vertices, but the loop doesn't seem to suffer from N+1 overruns.

@YeldhamDev YeldhamDev added this to the 4.3 milestone Dec 2, 2023
@YeldhamDev YeldhamDev added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 2, 2023
Copy link
Member

@lawnjelly lawnjelly left a comment

Choose a reason for hiding this comment

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

Good spot, must have missed this when we looked at #50404 .

@lawnjelly
Copy link
Member

Makes sense to me, would like the original author to weigh in if there was a reason for this. This is not something people tent to do by accident, most likely there was other code here before that makes this make sense..

Pouley is probably unlikely to reply as he's moved on to other work, but this was implementing something I suggested about removing unnecessary conversion between longhand vertex format (with duplicated verts) and vertex + index format, if I remember right. Nothing super complex going on here.

@akien-mga akien-mga added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Dec 2, 2023
@akien-mga akien-mga merged commit 6a10b99 into godotengine:master Dec 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Dec 5, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.1.

@yythlj
Copy link

yythlj commented Jan 22, 2024

fail_enable_phy
I meet the problem same in 4.2.1 windows

@YuriSizov
Copy link
Contributor

@yythlj Please open an issue with more details about your case.

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.4.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants