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 frame double-free #1368

Merged
merged 1 commit into from
Jun 12, 2024
Merged

Fix frame double-free #1368

merged 1 commit into from
Jun 12, 2024

Conversation

xxie24
Copy link

@xxie24 xxie24 commented Jun 6, 2024

Decrement refcnt and load the refcnt should be in 1 instruction in order to prevent data race in a multithreading environment.

Decrement refcnt and load the refcnt should be in 1 instruction in order to
prevent data race in a multithreading environment.
@nilfm99
Copy link
Collaborator

nilfm99 commented Jun 7, 2024

Hi, thanks for the contribution. I believe a race condition is still present with the fix, for example:

initially refcount is 1
thread A does fetch_decrement, brings it to 0
thread B does fetch_increment, brings it to 1
thread C does fetch_decrement, brings it back to 0
thread A proceeds to release it
thread C proceeds to release it

so I think we'll need locks to completely safeguard this, but I would agree that the race condition in the current code is way more likely to happen, so it may be good to do this in the interim.

@nilfm99 nilfm99 requested review from nilfm99 and kylophone June 7, 2024 00:04
Copy link
Collaborator

@nilfm99 nilfm99 left a comment

Choose a reason for hiding this comment

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

I'll be merging these changes in one or two days to leave time for other people to comment if they want to.

@nilfm99
Copy link
Collaborator

nilfm99 commented Jun 12, 2024

I'll merge this for now, as mentioned above I think there is still potential for data races but this is better than status quo. We'll revisit this later.

@nilfm99 nilfm99 merged commit 4c0e631 into Netflix:master Jun 12, 2024
8 checks passed
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