Skip to content

wolfsftp: report a failed remote write from SFTP put - #1182

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

wolfsftp: report a failed remote write from SFTP put#1182
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_10543

Conversation

@yosuke-wolfssl

Copy link
Copy Markdown
Contributor

Problem

wolfSSH_SFTP_Put() reports a successful upload after the server rejects a write. When wolfSSH_SFTP_SendWritePacket() returns a non-positive size for a non-retryable reason — chiefly a server FXP_STATUS != FX_OK, which surfaces as WS_SFTP_STATUS_NOT_OK — the code only consulted NoticeError() and dropped the error. The do/while (sz > 0) loop then ended normally, and STATE_PUT_CLOSE_REMOTE overwrote ret with the successful close result, so STATE_PUT_CLEANUP returned WS_SUCCESS.

Trigger: open a remote file successfully, then have the server fail the write because its filesystem is full or read-only. The application is told the transfer succeeded while the remote file is truncated or empty. Closes f-10543.

Fix (src/wolfsftp.c)

STATE_PUT_WRITE now records the failure and routes around the close that would mask it:

WLOG(WS_LOG_SFTP, "Error writing packet");
if (ssh->error == WS_SUCCESS) {
    ssh->error = (sz < 0) ? sz : WS_FATAL_ERROR;
}
ret = WS_FATAL_ERROR;
/* no remote close, it would overwrite ret */
state->handleSz = 0;
state->state = STATE_PUT_CLOSE_LOCAL;
break;
  • ssh->error is filled in only when unset, so a specific code such as WS_SFTP_BAD_HEADER survives; the return stays WS_FATAL_ERROR per convention.
  • state->handleSz = 0 makes STATE_PUT_CLOSE_REMOTE skip the close, the same mechanism the STATE_PUT_OPEN_REMOTE failure path already uses.
  • continue after the loop reaches cleanup with ret intact.

This mirrors wolfSSH_SFTP_Get(), which likewise skips its remote close on a hard read failure. The trade-off is deliberate: the server-side handle stays open until session teardown, and a local ret cannot survive a WS_WANT_READ from the close, since it is re-initialised on every entry.

Tests

test_SftpClientPutWriteStatusFail() reuses the existing in-memory client harness (SftpClientNewSession(), wolfSSH_TestChannelPutData()) to stage an FXP_HANDLE open reply and an FXP_STATUS failure for the only write, then asserts WS_FATAL_ERROR, WS_SFTP_STATUS_NOT_OK, and that putState was freed.

Verification

  • make check: 11 passed, 1 skipped, 0 failed.
  • Negative control: the test fails with the wolfsftp.c change reverted.
  • Preflight: gcc-13 -Werror, lint plus 6 configs clean.

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

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

Fixes a client-side SFTP upload correctness bug where wolfSSH_SFTP_Put() could incorrectly return success after a non-retryable remote write failure (e.g., server returns FXP_STATUS != FX_OK), and adds a regression test to ensure the failure is surfaced to callers via WS_FATAL_ERROR and ssh->error.

Changes:

  • Update STATE_PUT_WRITE in wolfSSH_SFTP_Put() to preserve the first non-retryable write error, avoid the remote close that could mask ret, and unwind via local close + cleanup while returning WS_FATAL_ERROR.
  • Add a unit test that simulates an FXP_HANDLE reply followed by an FXP_STATUS failure on the first write, asserting wolfSSH_SFTP_Put() returns WS_FATAL_ERROR and wolfSSH_get_error() reports WS_SFTP_STATUS_NOT_OK.

Reviewed changes

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

File Description
tests/unit.c Adds a regression unit test that simulates a server write-status failure during SFTP put and asserts correct error propagation and cleanup.
src/wolfsftp.c Ensures non-retryable write failures in SFTP put are recorded and returned, preventing later close logic from overwriting the failure.

💡 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 #1182

Scan targets checked: wolfssh-bugs, wolfssh-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.

Comment thread src/wolfsftp.c
Comment thread tests/unit.c
Comment thread tests/unit.c Outdated
Comment thread src/wolfsftp.c Outdated
Comment thread src/wolfsftp.c
- STATE_PUT_WRITE logs, sets ret to WS_FATAL_ERROR, clears
  state->handleSz and moves to STATE_PUT_CLOSE_LOCAL when
  wolfSSH_SFTP_SendWritePacket() returns a non-positive size and
  NoticeError() is false.
- ssh->error takes that return value, or WS_FATAL_ERROR for a size of
  zero, when ssh->error is still WS_SUCCESS.
- The write loop is followed by a continue when ret is not
  WS_SUCCESS.
- tests/unit.c gains WOLFSSH_TEST_SFTP_PUT, the SftpBuildReply()
  helper, and test_SftpClientPutWriteStatusFail(), which drives
  wolfSSH_SFTP_Put() over a staged handle reply and a write answered
  by an FXP_STATUS failure.

Issue: F-10543
Comment thread src/wolfsftp.c
Comment thread src/wolfsftp.c
Comment thread tests/unit.c
Comment thread tests/unit.c Outdated
Comment thread src/wolfsftp.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 #1182

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 4
4 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment thread src/wolfsftp.c
Comment thread tests/unit.c
Comment thread src/wolfsftp.c
Comment thread tests/unit.c
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