Skip to content

wolfsftp: keep the remote file when resuming a put - #1191

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_11659
Open

wolfsftp: keep the remote file when resuming a put#1191
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_11659

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Problem

Two defects, both leaving a corrupted file at the destination.

Client (wolfSSH_SFTP_Put, f-11659): resume mode restores the saved
offset and seeks the local source forward, but still opens the destination with
WOLFSSH_FXF_TRUNC. The prefix already uploaded is discarded and writes begin
at the nonzero offset, so a reput yields a zero-filled hole followed by the
tail, with no error returned.

Windows server (surfaced by the new test): WOLFSSH_FXF_WRITE | WOLFSSH_FXF_CREAT mapped to CREATE_ALWAYS, which truncates unconditionally,
and WOLFSSH_FXF_TRUNC was #if 0'd out. Any SFTP client writing at explicit
offsets loses the destination's contents. The POSIX server maps each flag
separately and is unaffected.

Fix (src/wolfsftp.c)

TRUNC is added only when the write offset is zero. Dropping it alone would
still hole a missing or short destination, and would newly allow stale trailing
bytes on a longer one, so a new STATE_PUT_STAT_REMOTE validates first. It runs
before the local open — where the offset is consumed (WFSEEK, or the
OVERLAPPED seed on Windows) — and does nothing unless the saved offset is
nonzero, so a plain put costs no extra round trip.

wolfSSH_SFTP_STAT() on the destination Action
size equals the saved offset resume, no TRUNC
size differs, or no size attribute restart from 0 with TRUNC
WS_SFTP_STATUS_NOT_OK, WS_PERMISSIONS restart from 0 with TRUNC
WS_WANT_READ / WS_WANT_WRITE retry in place
any other failure cleanup, error propagates

The Windows server disposition now separates the two cases:

if (reason & WOLFSSH_FXF_TRUNC)
    creationDisp = CREATE_ALWAYS;
else
    creationDisp = OPEN_ALWAYS;

WS_SFTP_PUT_STATE carries the returned attributes; it is defined only in
wolfsftp.c, so there is no header or ABI change.

Tests (tests/api.c)

test_wolfSSH_SFTP_PutResume() is the first coverage of wolfSSH_SFTP_Put().
Four cases, each checking the destination byte-for-byte: resume onto a matching
prefix, resume with the destination deleted, resume with an offset that
undershoots it, and a plain put over a longer destination. The resume case
stages a prefix unlike the source and expects it back untouched, so a silent
full re-upload fails the test. Excluded on Zephyr, which lacks the hosted
wrappers needed to stage the local source.

Not covered: the stat's permission-denied restart and its hard-error abort.
Both need a server that can be made to fail a stat on demand; staging a
permission failure through the filesystem would also fail the following open,
so the test would pass for the wrong reason.

Verification

  • Clean under gcc-13 -Werror across 6 configs; make check 11 passed,
    1 skipped, 0 failed; ASan + UBSan clean.
  • Windows MSVC Debug x64: api-test passes — it failed on the resume case
    before the server fix.
  • Four negative controls, each failing a different scenario: reverting the
    patch, disabling the stat state, disabling the size comparison, and never
    resuming (always restarting).

@yosuke-wolfssl yosuke-wolfssl self-assigned this Aug 24, 2026
Copilot AI lite review requested due to automatic review settings August 24, 2026 04:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a data-corruption bug in wolfSSH_SFTP_Put() when resuming an upload (reput): the destination file was previously opened with WOLFSSH_FXF_TRUNC unconditionally, which discarded the already-uploaded prefix and could silently produce a hole + tail.

Changes:

  • Add a new STATE_PUT_STAT_REMOTE step to validate that the remote destination size matches the saved resume offset; otherwise reset the offset and restart from 0.
  • Make remote open truncation conditional on starting offset (TRUNC only when offset is 0).
  • Add test_wolfSSH_SFTP_PutResume() to validate correct behavior across resume and non-resume scenarios against the in-process echoserver.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/wolfsftp.c Adds remote stat validation for resume offsets and makes truncation conditional on starting at offset 0 to prevent resumed-upload corruption.
tests/api.c Adds a new API-level regression test covering resumed put behavior and ensuring correct remote file contents.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1191

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.

Comment thread tests/api.c Outdated
- wolfSSH_SFTP_Put() adds WOLFSSH_FXF_TRUNC to the destination open
  only when the write offset is zero, and gains an openFlags local
  to build that value.
- A new STATE_PUT_STAT_REMOTE stats the destination before the local
  file is opened, whenever the saved offset is nonzero. It clears the
  offset unless the reported size matches it exactly, and also clears
  it when the stat returns WS_SFTP_STATUS_NOT_OK or WS_PERMISSIONS;
  other stat failures move to STATE_PUT_CLEANUP, and a want-read or
  want-write keeps the state for the next call.
- WS_SFTP_PUT_STATE carries the attributes that state reads.
- The Windows server open maps WOLFSSH_FXF_CREAT to OPEN_ALWAYS and
  reserves CREATE_ALWAYS for an open that also asked for
  WOLFSSH_FXF_TRUNC; the disabled TRUNCATE_EXISTING mapping is
  dropped.
- tests/api.c adds test_wolfSSH_SFTP_PutResume(), covering a resumed
  put onto a matching destination, a missing destination, a size
  mismatch, and a plain put over a longer destination. It builds
  where the hosted file wrappers are available.

Issue: F-11659
Comment thread tests/api.c Outdated

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1191

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
wolfSSL-Fenrir-bot dismissed their stale review August 24, 2026 05:47

Fenrir's latest completed scan found no issues; clearing the prior automated change request.

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.

4 participants