wolfsftp: keep the local file when resuming a get - #1216
Merged
Conversation
wolfSSL-Fenrir-bot
previously requested changes
Aug 31, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1216
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. Reported findings require changes before merge.
- The Windows local open in wolfSSH_SFTP_Get() passes OPEN_EXISTING when the write offset is nonzero and CREATE_ALWAYS otherwise, and drops FILE_APPEND_DATA from the desired access. A new DWORD creationDisp replaces the block-scoped desiredAccess. - That open reports INVALID_HANDLE_VALUE as WS_BAD_FILE_E and moves to STATE_GET_CLEANUP; the OVERLAPPED offset is set from gOfst on every open rather than only when resuming. - STATE_GET_LOOKUP_OFFSET clears a saved offset when the remote size STATE_GET_LSTAT stored in state->attrib is no larger than it, and again when the local destination does not hold exactly that many bytes. The destination stat overwrites state->attrib. - tests/api.c adds test_wolfSSH_SFTP_GetResume() and its sftpGetToCompletion() helper, six cases over the resume paths, built where the hosted file wrappers are available. - test_wolfSSH_SFTP_PutResume() and test_wolfSSH_SFTP_GetResume() drop the WOLFSSH_ZEPHYR k_sleep() block their bodies exclude. - .gitignore covers every wolfssh_*.tmp the api tests leave behind on an aborted run, replacing the known_hosts-only entry. Issue: F-12547
yosuke-wolfssl
force-pushed
the
fix/f_12547
branch
from
August 31, 2026 05:15
3d8e1e0 to
c57a905
Compare
yosuke-wolfssl
commented
Aug 31, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1216
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
wolfSSL-Fenrir-bot
dismissed
their stale review
August 31, 2026 05:35
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
ejohnstown
approved these changes
Sep 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
wolfSSH_SFTP_Get()opened the local destination withCREATE_ALWAYSon every Windows transfer, including a resumed one. The file was truncated to zero beforeWriteFile()resumed at the saved offset, so the already-downloaded prefix came back as a zero-filled gap — aregeton Windows silently corrupted the file it was meant to finish.Separately, neither platform checked the saved offset against the destination's real size. On POSIX
"ab"appends at the file's current end regardless ofgOfst, so a destination that was deleted, replaced, or shortened between the interrupt and theregetgot the remainder spliced in at the wrong position. The corruption is not Windows-only: on POSIX aregetonto a missing destination writes the remainder at offset 0, producing a 768-byte file holdingsrc[256..1024].Fix (
src/wolfsftp.c)STATE_GET_LOOKUP_OFFSETstats the destination when the saved offset is nonzero and clears the offset unless the reported size matches it exactly, restarting the transfer from zero. This mirrorsSTATE_PUT_STAT_REMOTE, added for the upload direction in 11659, and is not#ifdef-guarded — it corrects the POSIX and Harmony append paths too.STATE_GET_LOOKUP_OFFSETalso clears the saved offset when the remote size already instate->attribfromSTATE_GET_LSTATis less than or equal to it, so a shortened remote file cannot leave the local copy longer than its source. This mirrorsSTATE_PUT_LOOKUP_OFFSET.STATE_GET_OPEN_LOCALpassesOPEN_EXISTINGwhen the write offset is nonzero and reservesCREATE_ALWAYSfor offset zero.FILE_APPEND_DATAis dropped from the desired access:GENERIC_WRITEalready grants it and the write offset comes from theOVERLAPPED. The open now reportsINVALID_HANDLE_VALUEasWS_BAD_FILE_Erather than falling through on a staleret.Closes f-12547.
Tests
test_wolfSSH_SFTP_GetResume()intests/api.c, six cases over a 1024-byte source with a 256-byte offset:resume111101Staging a prefix that differs from the source is what separates a real resume from a re-download, and the comparison reads one byte past the expected size so a leftover tail fails.
Verification
make check7 passed / 2 skipped / 0 failed, includingscripts/sftp.testandget-put.test.api-testandunit-testboth exit 0.CREATE_ALWAYSrestored the resume case fails on Windows.Not in this PR
A failed local open jumps to
STATE_GET_CLEANUPwithout closing the remote handle. This is pre-existing on every branch — the siblingif (ret != 0)path does the same — and routing throughSTATE_GET_CLOSE_REMOTEwould overwriteretand surfaceWS_CLOSE_FILE_Einstead ofWS_BAD_FILE_E, so fixing both branches properly belongs in its own change.