Fix expected failing value for WFSEEK() - #1145
Conversation
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1145
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a portable way to interpret WFSEEK() results across platforms where seek returns a new file position (negative on failure), and updates call sites to use that check instead of comparing against 0.
Changes:
- Introduces
WFSEEK_SUCCESS()inwolfssh/port.hwith port-specific overrides plus a default fallback. - Updates
WFSEEK()result checks across core SSH/SFTP/SCP code and several examples/apps to useWFSEEK_SUCCESS(). - Adds/adjusts error handling paths when seek fails (close file / return error).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/port.h | Adds WFSEEK_SUCCESS() and default definition to standardize seek success checks across ports. |
| src/wolfsftp.c | Uses WFSEEK_SUCCESS() when seeking for resumed uploads. |
| src/wolfscp.c | Uses WFSEEK_SUCCESS() for _GetFileSize() and returns an error on seek failure. |
| src/ssh.c | Uses WFSEEK_SUCCESS() when seeking key files to determine size. |
| src/port.c | Uses WFSEEK_SUCCESS() for Harmony seek checks in wfopen(). |
| ide/Espressif/ESP-IDF/examples/wolfssh_echoserver/main/echoserver.c | Adds seek failure handling using WFSEEK_SUCCESS() when loading files/keys. |
| examples/tpmcertserver/tpmcertclient.c | Adds seek failure handling using WFSEEK_SUCCESS() when loading files. |
| examples/echoserver/echoserver.c | Adds seek failure handling using WFSEEK_SUCCESS() when loading files/keys. |
| examples/client/common.c | Updates DER/key blob loaders to use WFSEEK_SUCCESS() and handle seek failure. |
| apps/wolfsshd/wolfsshd.c | Adds seek failure handling using WFSEEK_SUCCESS() when reading files to buffers. |
| apps/wolfssh/common.c | Updates DER loader to use WFSEEK_SUCCESS() when seeking to end. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
wolfssh/port.h:127
- Casting the seek result to
intcan truncate large positive file positions (e.g.,long/off_t), potentially turning a successful seek into a negative value and falsely reporting failure. Prefer comparing in the native type (e.g.,(r) >= 0) or casting to a sufficiently wide signed type used by the port’s seek API.
#define WFSEEK_SUCCESS(r) ((int)(r) >= 0)
wolfssh/port.h:444
- Casting the seek result to
intcan truncate large positive file positions (e.g.,long/off_t), potentially turning a successful seek into a negative value and falsely reporting failure. Prefer comparing in the native type (e.g.,(r) >= 0) or casting to a sufficiently wide signed type used by the port’s seek API.
#define WFSEEK_SUCCESS(r) ((int)(r) >= 0)
src/wolfscp.c:2707
- On the
tmpSz < 0error path, the file position is left at EOF (after a successful seek), whereas the success path rewinds. To keep_GetFileSize()behavior consistent and reduce caller surprises, consider rewinding (or restoring the prior position) before returningWS_BAD_FILE_EwhenWFTELL()fails.
if (WFSEEK_SUCCESS(WFSEEK(fs, fp, 0, WSEEK_END))) {
tmpSz = WFTELL(fs, fp);
if (tmpSz < 0) {
return WS_BAD_FILE_E;
}
*fileSz = (word32)tmpSz;
WREWIND(fs, fp);
return WS_SUCCESS;
}
return WS_BAD_FILE_E;
Adds on to #1139. Adds
WFSEEK_SUCCESS()towolfssh/port.h, fixes issue where checking the return value ofWFSEEK()against 0 may fail erroneously due to Nucleus (NU_Seek()) and MPLAB Harmony (SYS_FS_FileSeek()) returning the new file position and only signaling failure with a negative value.Fixed existing checks that misread the return value (already tested the result, but with
!= 0or!= -1):src/ssh.c—wolfSSH_ReadKey_file()src/wolfsftp.c— resume seek inwolfSSH_SFTP_Put()src/port.c—wPwrite()/wPread()(Harmony)apps/wolfssh/common.c,examples/client/common.c—load_der_file()Added checks where the result was discarded
src/wolfscp.c—_GetFileSize()apps/wolfsshd/wolfsshd.c—getBufferFromFile()examples/echoserver/echoserver.cand the ESP-IDF copy —load_file(),LoadTpmSshKey()examples/client/common.c—readKeyBlob()examples/tpmcertserver/tpmcertclient.c—TpmCcLoadFile()tests/auth.c,tests/regress.c— file loadersWFTELL()negative-return guards - can return-1, and several callers cast straight toword32, turning an error into a 4 GB size