Skip to content
This repository was archived by the owner on Jan 5, 2026. It is now read-only.

Increase max size for signatures in speed.c#178

Merged
baentsch merged 3 commits intoopen-quantum-safe:OQS-OpenSSL_1_1_1-stablefrom
christianpaquin:cp-issue159
May 12, 2020
Merged

Increase max size for signatures in speed.c#178
baentsch merged 3 commits intoopen-quantum-safe:OQS-OpenSSL_1_1_1-stablefrom
christianpaquin:cp-issue159

Conversation

@christianpaquin
Copy link

Increases max signature sizes for speed tests to 35000 (enough for picnicl1fs). Test code is hard to read, but it uses the last value of lengths_list array to allocate the signature buffer in the OQS tests. We'll need to update this value as we add algs with larger sigs.

Fixes issue #159.

@christianpaquin christianpaquin requested a review from baentsch May 6, 2020 21:23
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

I'm dumbfounded. Thanks very much: It looks like this indeed fixes the error. A full speed run with this patch doesn't SIGSEGV any more. Something else with picnic must have changed, though, as the error couldn't be triggered any more (before applying this patch!) by running picnic alone (as was the case when I created the issue). Whatever, case closed!

Before merging, would you please remove the two references to OPENSSL_NO_CLEANUP to re-activate full testing (one in apps/speed.c and one in oqs_test/scripts/do_openssl-speed.sh)? Thanks in advance.

@christianpaquin
Copy link
Author

Something else with picnic must have changed, though, as the error couldn't be triggered any more (before applying this patch!) by running picnic alone (as was the case when I created the issue). Whatever, case closed!

The speed tool crashed for me at different point on different machines (sometimes during signing, sometime during verification). Probably depends on the state of the heap, which could vary I suppose from machine to machine, debug vs. release, etc.

Before merging, would you please remove the two references to OPENSSL_NO_CLEANUP to re-activate full testing (one in apps/speed.c and one in oqs_test/scripts/do_openssl-speed.sh)?

Will do.

@christianpaquin
Copy link
Author

@baentsch, in do_openssl-speed.sh, shouldn't there be another return code check after running the sig tests, just like with the KEMs:

if [ $? -ne 0 ]; then
   exit -1
fi

@baentsch
Copy link
Member

baentsch commented May 7, 2020

@christianpaquin No, such code would not be necessary: The last command run provides the exit code for the whole script: If it fails, the script fails (and vice versa). But it also shouldn't hurt.

@christianpaquin christianpaquin requested a review from baentsch May 7, 2020 15:29
Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

do_openssl-speed.sh redone anyway.

@dstebila
Copy link
Member

do_openssl-speed.sh redone anyway.

Currently Github is recording a conflict with do_openssl-speed.sh. Will someone be rebasing to resolve the conflict before merging?

@baentsch
Copy link
Member

Easiest in my eyes would be probably for @christianpaquin to just remove do_openssl-speed.sh completely from the PR (which I didn't suitably express in my obviously overly short comment above). Sorry for asking for this change in the first place (as it became unnecessary due to the testing overhaul).

This reverts commit 9c9a771.

Reverting changes to do_openssl-speed.sh as it now conflicts with testing overhaul PR.
@christianpaquin
Copy link
Author

Done.

@baentsch baentsch merged commit e2d6bd3 into open-quantum-safe:OQS-OpenSSL_1_1_1-stable May 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants