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

Mild race condition in secp256k1_context_preallocated_create #351

Open
Kixunil opened this issue Jan 4, 2022 · 8 comments · May be fixed by #359
Open

Mild race condition in secp256k1_context_preallocated_create #351

Kixunil opened this issue Jan 4, 2022 · 8 comments · May be fixed by #359

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Jan 4, 2022

Possible scenario:

  1. Assuming the context wasn't initialized yet
  2. Threads A and B loaded HAVE_PREALLOCATED_CONTEXT
  3. Thread A is suspended by the OS
  4. Thread B initializes the context and stores DONE
  5. Thread A is scheduled and swaps DONE for WORKING
  6. Thread C loads WORKING
  7. Thread A sees the mistake and stores DONE again
  8. Thread C can now continue

In this scenario C needlessly waited for A even though the context was initialized. I suggest to use compare exchange to avoid erroneous swapping in the first place.

There's also break missing so B always makes one more iteration of the loop and may even ironically become thread C.

@elichai
Copy link
Member

elichai commented Jan 4, 2022

Just want to note that this is only for the fuzzing configuration

@TheBlueMatt
Copy link
Member

Right, this just seems like "it could be more performant", not a specific bug, if I'm reading your description correctly, which isn't super relevant given its only used in fuzzing and presumably after initial startup things are "warm" and just stay DONE.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 4, 2022

Ah, missed that it's only in fuzzing. Not looking immediately suspicious could have some benefit for reviewers too but if nobody wants to bother feel free to close.

@TheBlueMatt
Copy link
Member

We could add a comment, I guess? But, really, std should expose Once even if we don't have the ability to park a thread - the code here is basically the same as the std Once, with the only difference being that we don't park threads and instead busy-loop to avoid using std.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 4, 2022

Once in std is actually quite involved. It uses an intrusive linked list to track waiting threads. I think it'd be pretty complicated to make usable without std.

@TheBlueMatt
Copy link
Member

Yes, that's my point, it does the same thing plus parks threads (with relevant tracking to wake them up again).

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jan 5, 2022

The code difference is so large that I'm not sure it'd be reasonable to abstract. And I generally love abstractions.

Anyway I think just changing swap to compare_exchange is better than a comment. :)

@apoelstra
Copy link
Member

I agree with changing swap to compare_exchange.

Philosophically I agree with Matt that "it doesn't matter" and also that it's somehow std's fault ;) but there's a straightforward solution so let's just take it.

@Kixunil Kixunil linked a pull request Jan 5, 2022 that will close this issue
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 a pull request may close this issue.

4 participants