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 race condition in NotePlayHandleManager #4966

Merged
merged 1 commit into from May 8, 2019

Conversation

Projects
None yet
2 participants
@PhysSong
Copy link
Member

commented Apr 29, 2019

Fixes #4956 by using a write lock when acquiring NotePlayHandles.
It should be superseded by #4443 or similar on master.

Fix race condition in NotePlayHandleManager
NotePlayHandleManager::acquire uses a read lock unless the pool is empty.
If two threads try to acquire NotePlayHandle simultaneously
when the value of s_availableIndex is 1, one thread may return null pointer.
Also, the ABA problem may occur when the acquire action and the release action
are done at the same time.

This commit prevents this by always using the write lock
when acquiring a NotePlayHandle.
@JohannesLorenz

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

I might be interested in reviewing this. Is it already ready to review, or do you plan further commits?

@JohannesLorenz JohannesLorenz added the core label May 2, 2019

@PhysSong

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

I might be interested in reviewing this. Is it already ready to review, or do you plan further commits?

I'm not going to add new commits unless there are some regressions.

@JohannesLorenz JohannesLorenz self-requested a review May 4, 2019

@JohannesLorenz

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

Code is stylistic and functionally OK and should fix the mentioned issues.

The commit description confused me:

  • in the mentioned case where two threads are calling the function and s_availableIndex is 1, one thread is doing an OOB read and only returns nullptr when you're lucky?
  • the ABA problem occurs when there's a comparison for something to still be the same (still be "A"). I see the race condition, but wouldn't call it an ABA problem (where is the mentioned comparison?)

Is there any way to also test the changes? Maybe an issue report showing how to reproduce this?

Btw... s_available[++s_availableIndex] = nph; in NotePlayHandleManager::release can still be called simultaneously by 2 threads. Imagine two threads enter here, and the order is

s_available_index is 0
Thread 1: ++s_available_index;
Thread 2: ++s_available_index;
Thread 1: s_available[2] = nph;
Thread 2: s_available[2] = nph;

Isn't this leaving a hole at s_available[1]? Should this be fixed?

@PhysSong

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Btw... s_available[++s_availableIndex] = nph; in NotePlayHandleManager::release can still be called simultaneously by 2 threads. Imagine two threads enter here, and the order is

s_available_index is 0
Thread 1: ++s_available_index;
Thread 2: ++s_available_index;
Thread 1: s_available[2] = nph;
Thread 2: s_available[2] = nph;

Isn't this leaving a hole at s_available[1]? Should this be fixed?

The operations on s_available_index are atomic.

static AtomicInt s_availableIndex;

So, this will happen instead:

s_available_index is 0
Thread 1: ++s_available_index; // gets 1 here because the operation is atomic
Thread 2: ++s_available_index; // gets 2 here because the operation is atomic
Thread 1: s_available[1] = nph;
Thread 2: s_available[2] = nph;
  • in the mentioned case where two threads are calling the function and s_availableIndex is 1, one thread is doing an OOB read and only returns nullptr when you're lucky?

That would be a better description.

  • the ABA problem occurs when there's a comparison for something to still be the same (still be "A"). I see the race condition, but wouldn't call it an ABA problem (where is the mentioned comparison?)

My description was wrong, but what I meant was:

  1. s_available_index was 0
  2. Thread 1: ++s_available_index;, gets 1
  3. Thread 2: s_available_index--;, gets 1
  4. Thread 2: NotePlayHandle * nph = s_available[1]; before writing
  5. Thread 1: s_available[1] = nph;
@JohannesLorenz

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

The operations on s_available_index are atomic.

Oh, I thought this was just atomic add, but yeah, it's fetch_add.

I'm for merging this. If you want, you can edit the commit message to clarify things.

@PhysSong PhysSong merged commit b9503a8 into LMMS:stable-1.2 May 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PhysSong PhysSong deleted the PhysSong:nphmgr branch May 8, 2019

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