Refactor All SetUp fucntions to use GetOpt and Handle new error codes / Standardization#260
Refactor All SetUp fucntions to use GetOpt and Handle new error codes / Standardization#260aidankeefe2022 wants to merge 4 commits into
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #260
Scan targets checked: wolfclu-bugs, wolfclu-src
Findings: 5
5 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
| switch (option) { | ||
| case ARG_FOUND_TWICE: | ||
| wolfCLU_LogError("Found duplicate argument"); | ||
| return WOLFCLU_FATAL_ERROR; |
There was a problem hiding this comment.
🔵 [Low] ARG_FOUND_TWICE returns without freeing key buffers · Resource leaks on error paths
The added case ARG_FOUND_TWICE returns WOLFCLU_FATAL_ERROR directly, leaking pwdKey, iv, and key which are XMALLOC'd before the parse loop. Every other exit path in this function calls wolfCLU_freeBins.
Fix: Call wolfCLU_freeBins(pwdKey, iv, key, NULL, NULL) before returning.
| switch (option) { | ||
| case ARG_FOUND_TWICE: | ||
| wolfCLU_LogError("Found duplicate argument"); | ||
| return WOLFCLU_FATAL_ERROR; |
There was a problem hiding this comment.
🔵 [Low] ARG_FOUND_TWICE returns without freeing dataBio · Resource leaks on error paths
The added case ARG_FOUND_TWICE returns directly, leaking dataBio which is opened at function entry before the loop. The normal exit frees it via wolfSSL_BIO_free(dataBio).
Fix: Free dataBio (and other BIOs) before returning, or set ret and fall through to the shared cleanup.
|
|
||
| case ARG_FOUND_TWICE: | ||
| wolfCLU_LogError("Found duplicate argument"); | ||
| return WOLFCLU_FATAL_ERROR; |
There was a problem hiding this comment.
🔵 [Low] ARG_FOUND_TWICE bypasses FreeTcpReady cleanup · Resource leaks on error paths
The added case ARG_FOUND_TWICE returns directly, bypassing the exit: label and its FreeTcpReady(&ready) (initialized unconditionally by InitTcpReady). The sibling WOLFCLU_HELP case correctly uses goto exit.
Fix: Use goto exit; instead of returning directly.
|
|
||
| case ARG_FOUND_TWICE: | ||
| wolfCLU_LogError("Found duplicate argument"); | ||
| return WOLFCLU_FATAL_ERROR; |
There was a problem hiding this comment.
🔵 [Low] ARG_FOUND_TWICE skips x509 cert cleanup · Resource leaks on error paths
The added case ARG_FOUND_TWICE returns directly, skipping the end-of-function cleanup; a duplicate argument detected after -signkey or -in (already processed in table order) may leak the keyIn/in BIOs.
Fix: Set ret = WOLFCLU_FATAL_ERROR; break; so the function's cleanup runs.
|
|
||
| case ARG_FOUND_TWICE: | ||
| wolfCLU_LogError("Found duplicate argument"); | ||
| return WOLFCLU_FATAL_ERROR; |
There was a problem hiding this comment.
🔵 [Low] ARG_FOUND_TWICE skips request cleanup · Resource leaks on error paths
The added case ARG_FOUND_TWICE returns directly, skipping cleanup that frees keyType, reqIn, and keyIn; a duplicate detected after -newkey/-in/-key may leak the already-allocated buffer/BIOs.
Fix: Set ret = WOLFCLU_FATAL_ERROR; break; so the end-of-function cleanup runs.
Rewriting Every Setup Function and Standardizing help function locations; Also slight refactor to wolfCLU_getOpt func
Why
Three main issues were pervasive before this change:
There was also a lot of legacy argument checking via the use of strcmp and checkForArgument calls that made understanding what was set when very difficult and adding new functionality much harder than in the modern functions that used getOpt. Moving the help functions just made the code base consistent in style.
Benefits