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

No if xfree #7839

Merged
merged 12 commits into from
Aug 7, 2024
Merged

No if xfree #7839

merged 12 commits into from
Aug 7, 2024

Conversation

bandi13
Copy link
Contributor

@bandi13 bandi13 commented Aug 6, 2024

Remove 'if(ptr)' check before 'XFREE(ptr)'

@bandi13 bandi13 requested a review from SparkiDev August 6, 2024 14:59
@bandi13 bandi13 self-assigned this Aug 6, 2024
@bandi13
Copy link
Contributor Author

bandi13 commented Aug 6, 2024

retest this please. Just a random test failure:

Client message: hello wolfssl!
I hear you fa shizzle!
trying server command line[1193]: SuiteTest -v 4 -l TLS13-AES128-GCM-SHA256 -2 -p 0 
repeating test without extended master secret
trying client command line[1194]: SuiteTest -v 4 -l TLS13-AES128-GCM-SHA256 -F 1 -2 -n -p 36085 
FAIL scripts/unit.test (exit status: 1)
<<<
Error: Test runtime=91sec. Make Check RESULT = 2
Exiting with status: 1
Script ran for 269 seconds.
script returned exit code 1

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Please make sure all XFREE macros definitions have the if() check and clearly document this in that area of the headers. Its already a rule and I'm glad we are getting consistent with this, but let's document all #define XFREE examples.

@bandi13 bandi13 assigned wolfSSL-Bot and unassigned bandi13 Aug 6, 2024
dgarske
dgarske previously approved these changes Aug 6, 2024
src/ssl.c Outdated Show resolved Hide resolved
src/x509.c Outdated Show resolved Hide resolved
wolfcrypt/src/evp.c Outdated Show resolved Hide resolved
wolfcrypt/src/sp_arm32.c Show resolved Hide resolved
@bandi13 bandi13 removed their assignment Aug 7, 2024
@dgarske dgarske requested review from SparkiDev and douzzer August 7, 2024 16:58
Copy link
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

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

quantum-safe-wolfssl-all-clang-tidy found something to complain about:

[quantum-safe-wolfssl-all-clang-tidy] [22 of 32] [a31d8c5ce7]
    configure...   real 0m26.333s  user 0m11.293s  sys 0m16.875s
    build...d350ba6c41 (<[email protected]> 2024-08-06 10:44:59 -0400 7938)     XFREE(s1, key->heap, DYNAMIC_TYPE_DILITHIUM);
/tmp/tmp.4346_12509/wolfssl_test_workdir.35142/wolfssl/wolfcrypt/src/dilithium.c:7938:5: warning: Access to field 'heap' results in a dereference of a null pointer (loaded from variable 'key') [clang-analyzer-core.NullDereference]
7938 |     XFREE(s1, key->heap, DYNAMIC_TYPE_DILITHIUM);
|     ^
[...]

Copy link
Contributor

@douzzer douzzer left a comment

Choose a reason for hiding this comment

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

clang-tidy-asn-template-sp-all-small-stack found a similar defect in rsa.c:

[clang-tidy-asn-template-sp-all-small-stack] [32 of 32] [a31d8c5ce7]
    configure...   real 0m26.067s  user 0m11.164s  sys 0m16.735s
    build...d350ba6c41 (<[email protected]> 2024-08-06 10:44:59 -0400 5084)     XFREE(p, key->heap, DYNAMIC_TYPE_RSA);
/tmp/tmp.4346_12509/wolfssl_test_workdir.35142/wolfssl/wolfcrypt/src/rsa.c:5084:5: warning: Access to field 'heap' results in a dereference of a null pointer (loaded from variable 'key') [clang-analyzer-core.NullDereference]
5084 |     XFREE(p, key->heap, DYNAMIC_TYPE_RSA);
|     ^
[...]

@douzzer douzzer assigned bandi13 and unassigned douzzer Aug 7, 2024
@douzzer douzzer dismissed SparkiDev’s stale review August 7, 2024 22:07

all comments addressed

@douzzer douzzer merged commit 92952a5 into wolfSSL:master Aug 7, 2024
122 checks passed
@bandi13 bandi13 deleted the noIfXFREE branch August 9, 2024 18:23
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.

5 participants