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 468 #474

Closed
wants to merge 3 commits into from

Conversation

@rpavlik
Copy link
Contributor

commented Sep 9, 2019

Fixes #468 by removing use of pointers after an invalidation-causing action (CopyEntity).

I briefly reviewed the other code in this massive switch, and the rest (besides helix and revolve) appears to be safe: there are a few other places where the address of a list element is taken, but it's either done in the tightest loop and not used after the invalidation, and/or something like hEntity he = e->h; e = NULL; is done, where then SK.GetEntity(he) is used after in place of e. This seems sufficient, if a bit overkill: it's putting in a log(n) lookup by handle where there's already a good-enough index available (good enough that it's being used to control iteration, anyway.)

The last commit here is a matter of style preference: can't use an invalidated pointer if you don't take one in the first place. :) Take or leave, no worries.

The middle commit highlights some things I got confused about when reading the code, as well as adds the comments that got missed due to the feature being in-flight when I added them.

rpavlik added 3 commits Sep 9, 2019
Fix use-after-free in revolve and helix.
Pointers get invalidated if a list has to be re-allocated.
CopyEntity can do that.
@rpavlik

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Looks like some of the CI issues at least are issues getting FreeType cloned.

@whitequark

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Merged, thanks.

@whitequark whitequark closed this Sep 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.