-
Notifications
You must be signed in to change notification settings - Fork 122
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
Update pairwise consistency test failures to support gracefully continiung #2201
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2201 +/- ##
==========================================
+ Coverage 79.05% 79.06% +0.01%
==========================================
Files 612 612
Lines 106222 106222
Branches 15008 15009 +1
==========================================
+ Hits 83973 83988 +15
+ Misses 21597 21583 -14
+ Partials 652 651 -1 ☔ View full report in Codecov by Sentry. |
#else | ||
return 0; | ||
#endif | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this affect any unreachable-code analysis? As the return 0;
could be seen as unreachable due to AWS_LC_FIPS_failure
triggering an abort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory yes, but nothing seems to be catching the issue. Looks like GCC removed support for the check a while ago https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82100#c1, Clang has an option but we don't have it turned on https://clang.llvm.org/docs/DiagnosticsReference.html#wunreachable-code.
I enabled that option for clang and it correctly flags the exit(1)
is unreachable after the abort. But it doesn't catch this new unreachable code. Reading the bug reports it looks like detecting unreachable code is a hard problem and very dependent on compiler versions and optimizations.
Debug build:
bcm.c:398:5: error: code will never be executed [-Werror,-Wunreachable-code]
398 | exit(1);
| ^~~~
Release build didn't find anything else with Clang.
53c4de5
to
b8c17f6
Compare
b8c17f6
to
c51d1fb
Compare
#2195 took care of the ML-KEM change, the lower function returns the error from the PCT and in the event that the AWS_LC_FIPS_failure returns it returns the failure result to the caller. |
Description of changes:
Currently AWS_LC_FIPS_failure always aborts the process and it is impossible to continue after calling that function. In a future change this will be optional and AWS-LC will not abort and the overall API call should return a failure. This change updates the current pairwise consistency test spots to theoretically return a failure.
Call-outs:
This PR does not change the behavior, if there is a PWCT failure the process still aborts.
Testing:
With the current AWS_LC_FIPS_failure implementation it is impossible to test this change.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.