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

[WIP] Fix RID get_ptr_by_index access to freed objects #49501

Closed
wants to merge 1 commit into from

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented Jun 11, 2021

RID_Alloc get_ptr_by_index was allowing access to objects that had previously been freed, causing crashes in the BVH.

This PR changes the function to iterate by allocation id rather than index, and provides protection against access to freed allocations.

Fixes #44973

Notes

  • Thanks to extensive debugging by @pouleyKetchoupp the issue was tracked down to access to freed memory
  • This is a rough PR for discussion as I need to check it with pouley and reduz, and reduz may want to make / decide on the final PR. In particular I'm unsure whether the validator_chunks index used should be p_index or idx, I'm just guessing by reverse engineering the existing code.
  • There are probably multiple ways of fixing this.
  • Let me know of preferences for different names for functions etc.

Update

This version is changed to iterate through the list by allocation ID rather than index .. it doesn't matter the order for the BVH, and I suspect the index is semi random anyway, and iterating by alloc_id is more efficient and simpler.

While we could just check for nullptr and access up to max_alloc sequentially, it is inefficient if there are a lot of invalid allocations in the list, because of the spinlock .. so I'm trying a single function to move an iterator on to the next valid alloc_id.

  • We can alternatively iterate by index if desired, it isn't hugely different.
  • The other alternative is to maintain a separate list of used elements, similar to the free list, but this may be overkill for the use case (although we should consider whether this would be generally useful functionality for RID_PtrOwners).
  • Note that this implementation does a fair amount of naive silliness compared to having a used_list, like if there are 512 members in the scenario minimum chunk size, it will iterate through all 512, even if only 1 is active.

@lawnjelly lawnjelly requested review from a team as code owners June 11, 2021 02:37
RID_Alloc get_ptr_by_index was allowing access to objects that had previously been freed, causing crashes in the BVH.

This PR changes the function to iterate by allocation id rather than index, and provides protection against access to freed allocations.
@lawnjelly
Copy link
Member Author

Just to keep this updated, this bug is still active. This PR does fix it - we discussed it with @reduz at the time and if I remember correctly he expressed an interest in fixing it himself in a slightly different way. So may need some prodding to remember to fix this.

@lawnjelly
Copy link
Member Author

Superseded by #57655 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage of freed memory in DynamicBVH::optimize_incremental(int)
4 participants