SCP: let the application start the transfer - #1231
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new public API entry point is not covered by tests, and the repository already has SCP-related tests that should be extended to lock in the new retry/completion contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR exposes a server-side SCP transfer driver (wolfSSH_SCP_accept()) so applications that bind an "exec scp ..." command to a channel can initiate and drive the SCP transfer themselves, instead of relying on wolfSSH_accept()’s WS_SCP_INIT re-entry behavior.
Changes:
- Added a new public server-side API:
wolfSSH_SCP_accept(WOLFSSH* ssh)inwolfssh/wolfscp.h. - Implemented
wolfSSH_SCP_accept()as a wrapper aroundDoScpRequest()that normalizes completion toWS_SCP_COMPLETEand surfacesWS_WANT_READ/WS_WANT_WRITEfor non-blocking retry loops. - Cleared stale
WS_WANT_*error state on entry to match re-entrant retry behavior used elsewhere.
File summaries
| File | Description |
|---|---|
| wolfssh/wolfscp.h | Adds the public API declaration and documents intended server-side usage/contract. |
| src/wolfscp.c | Implements wolfSSH_SCP_accept() wrapper logic around DoScpRequest() with non-blocking retry semantics. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
An application that binds an "scp ..." command to a channel itself has no way to run the transfer; wolfSSH_accept() did it through a WS_SCP_INIT re-entry only that state machine can drive. - add wolfSSH_SCP_accept(), a wrapper over DoScpRequest() reporting WS_SCP_COMPLETE for any non-negative result, as accept() does - a receive-side want reaches the wrapper as a generic error with the want in ssh->error, so report the want itself and let the caller retry - state that resume contract beside the prototype, and clear a stale want on entry the way the other re-entrant entry points do
d587f6c to
e90eed8
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1231
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wolfSSH_SCP_accept() reported any want held in ssh->error in place of the DoScpRequest() result. A short send that SendChannelData() accepts leaves WS_WANT_WRITE there with nothing clearing it on a later flush, so a terminal result came back as retryable and the retry re-entered the state machine, re-sending an abort confirmation or reading past a bad message. - Substitute the want only when the result is WS_FATAL_ERROR and the want is WS_WANT_READ, the one case GetInputData() hides by value. - Leave write wants alone; the SCP state machine already returns them.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1231
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.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
An application that binds an "scp ..." command to a channel itself has no
way to run the transfer; wolfSSH_accept() did it through a WS_SCP_INIT
re-entry only that state machine can drive.
WS_SCP_COMPLETE for any non-negative result, as accept() does.
a stale want on entry the way the other re-entrant entry points do.