add initial SFTP benchmark collection scripts - #784
JacobBarthelmeh wants to merge 7 commits into
Conversation
adec8e2 to
ea11966
Compare
ea11966 to
75ac463
Compare
75ac463 to
9693152
Compare
9693152 to
9e0f9ed
Compare
…SSH results, up timeout, remove password auth option with test
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #784
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 1 of 1 in-scope changed file(s) opened by the reviewer
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
|
|
||
| #if !defined(WOLFSSH_NO_TIMESTAMP) && !defined(USE_WINDOWS_API) &&\ | ||
| defined(EXAMPLE_SFTP_BENCHMARK) | ||
| ret = WFOPEN(NULL, &f, fullpath, "rb"); |
There was a problem hiding this comment.
Benchmark sizes the remote path via local fopen instead of the transferred source file · Logic errors
fullpath is the remote SFTP path, but it is opened on the local filesystem. For PUT the size comes from the remote destination and not from local. Any remote path that does not also exist locally returns WS_BAD_FILE_E and aborts the transfer. The benchmark only works on loopback with a pre-copied file.
Suggested fix: For PUT, take the size from the local source file local. For GET, take it after the transfer from local or from the remote file attributes. Never fail the transfer because the size lookup failed.
| return WS_BAD_FILE_E; | ||
| } | ||
| longBytes = (word32)WFTELL(NULL, f); | ||
| WREWIND(NULL, f); |
There was a problem hiding this comment.
FILE handle opened for size measurement is never closed on the success path · Resource leaks
After WFTELL/WREWIND, f is never closed with WFCLOSE. Each benchmarked autopilot transfer leaks one open FILE stream and its file descriptor.
Suggested fix: Call WFCLOSE(NULL, f) right after the size is read, since the stream is not used again.
Basis: C11 7.21.5.1: fclose flushes and closes the stream and releases its resources; an unclosed stream is only released at program exit.
| #if !defined(WOLFSSH_NO_TIMESTAMP) && !defined(USE_WINDOWS_API) &&\ | ||
| defined(EXAMPLE_SFTP_BENCHMARK) | ||
| ret = WFOPEN(NULL, &f, fullpath, "rb"); | ||
| if (ret != 0 || f == WBADFILE) return WS_BAD_FILE_E; |
There was a problem hiding this comment.
Early returns in benchmark block leak the SFTP name list from doBuildRemotePath · Resource leaks on error paths
When doBuildRemotePath has allocated name, the new return WS_BAD_FILE_E paths (open failure and seek failure) skip wolfSSH_SFTPNAME_list_free(name), which leaks the list.
Suggested fix: Free name with wolfSSH_SFTPNAME_list_free before each new early return, or send these failures to a common cleanup path.
Related known findings (similar but distinct; listed for context, not part of this finding)
- F-10551 (open): File/function: candidate is in sftpclient.c/doAutopilot vs. F-10551 in wolfsftp.c/wolfSSH_SFTP_RecvOpenDir — different files and functions. Faulting operation: both are early error-return paths that skip freeing/unlinking an already-allocated list-type resource (SFTP name list vs. directory-list node) before returning an error code, so the general pattern is closely related. Root cause: 'new/existing error return bypasses required cleanup for a previously allocated resource' is shared, though the
No description provided.