Call Keccak_(X4_)Dispatch with pthread_once

[zxjtan]

Fixes #1548.

Call Keccak_(X4_)Dispatch using pthread_once to ensure setting of global function pointers is atomic and done only once.

• Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? NO
• Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? NO

[dstebila]

@zxjtan will you be continuing to work on this?

[zxjtan]

@dstebila I have no experience writing Win32 code, but I can try my hand at making it cross-platform if there are reviewers who can check that I am writing it idiomatically.

[baentsch]

@zxjtan OQS already has code parts that are not optimized for Win32. In other words, it'd be beneficial to see liboqs improve "just" for pthread environments (as is the goal with this PR) rather than wait for a good Win32 solution to the problem. If a Windows user runs into #1548 we can revisit. What should happen first/now, though, is that this PR passes CI. Do you have time to get this resolved?

[zxjtan]

@baentsch Sure, I can gate the pthread functionality and pass the CI quite soon.

[zxjtan]

CI still runs into undefined reference to symbol 'call_once@@GLIBC_2.28', any tips on how to proceed?

[baentsch]

CI still runs into undefined reference to symbol 'call_once@@GLIBC_2.28', any tips on how to proceed?

Well, that depends on which lib is supposed to hold this symbol: It seems, glibc2.28 doesn't/isn't C11 compliant. What about using pthreads (i.e., pthread_once instead? Then linking in pthreads lib should get this to work.

[zxjtan]

CI still runs into undefined reference to symbol 'call_once@@GLIBC_2.28', any tips on how to proceed?

Well, that depends on which lib is supposed to hold this symbol: It seems, glibc2.28 doesn't/isn't C11 compliant. What about using pthreads (i.e., pthread_once instead? Then linking in pthreads lib should get this to work.

No dice, build still fails with undefined reference to symbol 'pthread_once@@GLIBC_2.2.5'. However, in both the pthread_once and call_once cases, this is caused by error adding symbols: DSO missing from command line. Do I have to change something in the CMake files to make it link?

[baentsch]

Do I have to change something in the CMake files to make it link?

I'd say so: That's what I meant with

Then linking in pthreads lib should get this to work.

Sorry for being imprecise.

So, I'd think one option may be to lift the "pthreads-availability" test logic you correctly built in to the source code to cmakevariable level (such as to then add the lib there -- or not, if unavailable).

[baentsch]

Code LGTM. The only slight concern I have is that this seems to add pthreads as a requirement to liboqs (and hence, its users). Have you tested that this new code works OK if used by a downstream project, @zxjtan ?

Thinking that through, what about adding CI to liboqs automatically validating this property with downstream projects we consider essential, @dstebila ? Or would this create a hen-and-egg situation where downstream projects' status (of not being ready for an liboqs update) can break liboqs CI and hence stop the whole procedure? Let me create a separate discussion for this...

[zxjtan]

The only slight concern I have is that this seems to add pthreads as a requirement to liboqs

I don't think it does. I think of it not as a requirement but an environment fix: if you are running on a platform with threading, and if that threading model is pthreads, this PR provides a fix. Otherwise the code will not #include <pthread.h> because you're on something else like Win32 and you can submit a fix yourself, or you have no threading at all and there is nothing to fix.

Have you tested that this new code works OK if used by a downstream project, @zxjtan ?

Yes, I have used this code in a project and it fixes the race condition.

[baentsch]

LGTM. Thanks for the contribution! @dstebila This is one more item to be added to the "Windows ToDo" list (as and when we start it per https://github.com/orgs/open-quantum-safe/discussions/1583.

[Martyrshot]

This looks good to me too! Taking a brief look at the Win32 API the fix will likely look very similar, but it seems the biggest obstacle will be finding someone with a Win32 machine to implement the fix :-)

[baentsch]

@zxjtan Thanks again for the contribution; it turned out it causes a failure in one downstream integration: https://app.circleci.com/pipelines/github/open-quantum-safe/oqs-demos/1046/workflows/e474db0c-43e2-4628-b634-23b9f3b5d95a/jobs/5979. Does this ring a bell? At first glance this should work as -lpthread is clearly configured in.... Edit/correct: -lpthread is requested to be configured in but the final openssl link command doesn't contain that instruction... Hmm.....

[zxjtan]

@zxjtan Thanks again for the contribution; it turned out it causes a failure in one downstream integration: https://app.circleci.com/pipelines/github/open-quantum-safe/oqs-demos/1046/workflows/e474db0c-43e2-4628-b634-23b9f3b5d95a/jobs/5979. Does this ring a bell? At first glance this should work as -lpthread is clearly configured in.... Edit/correct: -lpthread is requested to be configured in but the final openssl link command doesn't contain that instruction... Hmm.....

The sed in this line might have something to do with it?

[baentsch]

The sed in this line might have something to do with it?

I don't think so: This doesn't change (presence of) -lpthread but just adds liboqs. HOWEVER, looking further into the build log, this line is key, I'd gather:

./config --prefix=/opt/oqs-openssl-quic/.openssl no-shared no-threads

This configures the OpenSSL installation within quic-nginx to be built without pthreads. Then, in turn, the open symbols in liboqs cannot resolve as liboqs is (needed to be) linked into openssl111.

I see two ways out of this:

1. Upgrade the nginx-quic demo to use OpenSSL3 (as per replace oqs-openssl111 oqs-demos#182)
2. Disable pthreads usage in the liboqs build for this demo

For 1) I am out of my depth (having to ask the original contributor @igorbarshteyn for help/advice); 2) would require a cmake variable in liboqs to disable the use of pthreads.

Any other options? We could of course also completely discontinue support of the QUIC demo as it is based on unsupported software (OpenSSL111). The more I think about this, the more I like it. Several people have voiced willingness to help on open-quantum-safe/oqs-demos#182 but nothing has been forthcoming. And OpenSSL111 by now is EOL. Thus I think we should sunset this PQ-OpenSSL1-QUIC integration.