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

20240814-debug-trace-errcodes-MP #7875

Merged
merged 3 commits into from
Aug 22, 2024

Conversation

douzzer
Copy link
Contributor

@douzzer douzzer commented Aug 15, 2024

move several MP error codes from wolfssl/wolfcrypt/sp_int.h, wolfssl/wolfcrypt/tfm.h, and wolfssl/wolfcrypt/integer.h, to wolfssl/wolfcrypt/error-crypt.h, harmonizing their names and numbers.

wolfssl/wolfcrypt/error-crypt.h: add WC_FIRST_E.

wolfcrypt/src/error.c: add MP error code strings.

wolfssl/error-ssl.h: add WOLFSSL_FIRST_E and WOLFSSL_LAST_E.

wolfcrypt/test/test.c: update error_test() for new error code layout, refactoring the "missing" check.

src/internal.c: use WC_FIRST_E and WC_LAST_E in wolfSSL_ERR_reason_error_string().

src/ssl.c: fix wolfSSL_ERR_GET_REASON() to identify in-range error codes using WC_FIRST_E, WC_LAST_E, WOLFSSL_FIRST_E, and WOLFSSL_LAST_E.

wolfcrypt/src/ecc.c: fix 2 identicalInnerCondition's.

note, refactoring the sp error codes as enums adds helpful additional error checking by the compiler, and also helped cppcheck uncover the benign-but-true identicalInnerConditions.

tested with wolfssl-multi-test.sh ... super-quick-check int-math all-enable-fastmath with the latter 2 tweaked to add --enable-debug-trace-errcodes.

src/pk.c Outdated Show resolved Hide resolved
wolfssl/wolfcrypt/sp_int.h Outdated Show resolved Hide resolved
@douzzer douzzer force-pushed the 20240814-debug-trace-errcodes-MP branch from 6ca1981 to 558d3c1 Compare August 17, 2024 00:54
@douzzer douzzer requested a review from SparkiDev August 17, 2024 00:57
SparkiDev
SparkiDev previously approved these changes Aug 19, 2024
…wolfcrypt/tfm.h, and wolfssl/wolfcrypt/integer.h, to wolfssl/wolfcrypt/error-crypt.h, harmonizing their names and numbers.

wolfssl/wolfcrypt/error-crypt.h: add WC_FIRST_E.

wolfcrypt/src/error.c: add MP error code strings.

wolfssl/error-ssl.h: add WOLFSSL_FIRST_E and WOLFSSL_LAST_E.

wolfcrypt/test/test.c: update error_test() for new error code layout, refactoring the "missing" check.

src/internal.c: use WC_FIRST_E and WC_LAST_E  in wolfSSL_ERR_reason_error_string().

src/ssl.c: fix wolfSSL_ERR_GET_REASON() to identify in-range error codes using WC_FIRST_E, WC_LAST_E, WOLFSSL_FIRST_E, and WOLFSSL_LAST_E.

sp_int.h: provide for WOLFSSL_DEBUG_TRACE_ERROR_CODES, and refactor MP error codes as enums, for consistency with other error codes.

wolfcrypt/src/ecc.c: fix 2 identicalInnerCondition's.
@douzzer douzzer force-pushed the 20240814-debug-trace-errcodes-MP branch 5 times, most recently from 77c5aab to 6a227c8 Compare August 21, 2024 03:50
@douzzer douzzer force-pushed the 20240814-debug-trace-errcodes-MP branch from 6a227c8 to 2448d48 Compare August 21, 2024 04:37
@douzzer douzzer requested a review from SparkiDev August 21, 2024 05:14
Copy link
Contributor

@bandi13 bandi13 left a comment

Choose a reason for hiding this comment

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

Good call on moving the error codes to 'error-crypt.h'. I still think since we are changing the values of MP constants we should collapse MP_MEM to MEMORY_E and MP_VAL to BAD_FUNC_ARG. I do understand the concern of collapsing error codes causing potential problems.

@SparkiDev
Copy link
Contributor

MP_VAL at least gives us the information that the error was in the maths.
Typically we've used a unique error code for every situation but thankfully we've used BAD_FUNC_ARG for bad parameters.
Let's leave MP_VAL for the time being. I think code checks explicitly for these MP error codes.

@SparkiDev SparkiDev merged commit e99bbf9 into wolfSSL:master Aug 22, 2024
123 of 125 checks passed
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.

3 participants