Propagate only WANT_WRITE in DoKexInit - #1278
Conversation
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1278
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 1 of 2 in-scope changed file(s) opened by the reviewer; not opened: tests/regress.c
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
There was a problem hiding this comment.
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.
Copilot review overview
Review effort: Lite
Findings: 1
Open (5)
retunconditionally based onssh->error. Ifretalready contains a real… · Newssh->errorbeing specifically attributable toSendKexInit(). A… · NewKEXDH_REPLY_REGRESS_KEX_ALGO. If this macro isn’t enabled in… · NewBuildKexInitPayload()succeeded. If it returns 0 (or an otherwise… · NewWS_WANT_WRITE, it would be valuable to add a case where… · New
What changed in this PR
Updates DoKexInit() to only propagate WS_WANT_WRITE back to the caller (avoiding failures due to unrelated stale ssh->error values) and adds regression coverage around stale errors and blocked reply sends.
Changes:
- Limit
DoKexInit()error propagation toWS_WANT_WRITEonly. - Add a regression helper and test cases to validate stale
ssh->errorhandling and blocked send behavior. - Wire the new regression test into
main()under an existing compile-time guard.
| File | Description |
|---|---|
| tests/regress.c | Adds a regression harness/test to ensure stale ssh->error values don’t break peer rekey, while still reporting blocked reply sends. |
| src/internal.c | Changes DoKexInit() to propagate only WS_WANT_WRITE instead of any non-zero ssh->error. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
fe9c874 to
c99ef2e
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1278
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
philljj
left a comment
philljj
left a comment
There was a problem hiding this comment.
merge conflict in tests/regress.c (from merging the terrapin PR probably)
c99ef2e to
1ebee2e
Compare
DoKexInit() reports a blocked KEXINIT reply to its caller. Take it from SendKexInit()'s return, not ssh->error, which holds whatever an earlier call left there: a WS_WINDOW_FULL from SendChannelData() failed the peer's rekey, and a WS_WANT_WRITE whose send had finished came back. - tests/regress.c runs DoKexInit() with WS_SUCCESS, WS_WINDOW_FULL, WS_CHAN_RXD and WS_WANT_WRITE left in ssh->error, its reply sent, blocked, failed, or not owed, and checks the KEXINIT is consumed. - The test needs only the server: REGRESS_SERVER_KEY_PATH moves out of the KEXDH_REPLY_REGRESS_KEX_ALGO block. Issue: F-14394
1ebee2e to
f2fd963
Compare
DoKexInit() takes a blocked KEXINIT reply from SendKexInit()'s return,
not ssh->error, so a stale WS_WINDOW_FULL no longer fails the peer's
rekey and a stale WS_WANT_WRITE is not reported again.
WS_WINDOW_FULL, WS_CHAN_RXD and WS_WANT_WRITE, with the reply sent,
blocked, failed, or not owed. The test runs in server-only builds.
Issue: F-14394