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

Forward-declare OQS_SIG in sig_stfl.h #1820

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Forward-declare OQS_SIG in sig_stfl.h #1820

merged 1 commit into from
Jun 11, 2024

Conversation

SWilson4
Copy link
Member

When an application (e.g., oqs-provider) includes <oqs/sig.h>, the following happens:

  • sig.h includes oqs.h at the top of the file.
  • oqs.h includes sig.h, but this include is skipped because of the OQS_SIG_H include guard.
  • oqs.h includes sig_stfl.h.
  • sig_stfl.h references OQS_SIG, which has not yet been defined.

This breaks the application's build: see #1815, open-quantum-safe/oqs-provider#427, and open-quantum-safe/oqs-demos#281.

This PR resolves the issue by forward-declaring the OQS_SIG type in sig_stfl.h. This fixes the failing oqs-provider build: see the downstream tests before and after.

I don't believe that the forward-declaration violates any C programming rules / best practices, but it would be great if someone with more experience could confirm this.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider will also need to be ready for review and merge by the time this is merged.)

@SWilson4 SWilson4 marked this pull request as ready for review June 11, 2024 16:27
@SWilson4 SWilson4 requested a review from dstebila as a code owner June 11, 2024 16:27
Copy link
Contributor

@cothan cothan left a comment

Choose a reason for hiding this comment

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

Great work! Just the DCO need to be fixed.

@SWilson4 SWilson4 force-pushed the sw-fix-stfl-include branch from f95aa27 to 2ae0b59 Compare June 11, 2024 18:53
@SWilson4 SWilson4 merged commit 39688e9 into main Jun 11, 2024
48 of 51 checks passed
@cothan cothan deleted the sw-fix-stfl-include branch June 11, 2024 19:00
@ashman-p
Copy link
Contributor

@SWilson4 Thanks for looking at this and resolving it.
I did also find that an alternative resolution but i prefer this fix.

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.

4 participants