Mild race condition in secp256k1_context_preallocated_create

[Kixunil]

Possible scenario:

0. Assuming the context wasn't initialized yet
1. Threads A and B loaded HAVE_PREALLOCATED_CONTEXT
2. Thread A is suspended by the OS
3. Thread B initializes the context and stores DONE
4. Thread A is scheduled and swaps DONE for WORKING
5. Thread C loads WORKING
6. Thread A sees the mistake and stores DONE again
7. 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]

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

[TheBlueMatt]

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]

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]

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]

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]

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

[Kixunil]

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]

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.