diff --git a/src/agent.c b/src/agent.c index ba861811..7f8d5fe7 100644 --- a/src/agent.c +++ b/src/agent.c @@ -1777,11 +1777,15 @@ int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh) } if (ret == WS_SUCCESS) { + word32 flushes = ssh->txFlushCount; + recordError = 1; ret = SendChannelOpenSession(ssh, newChannel); - if (ret < WS_SUCCESS - && ret != WS_WANT_WRITE && ret != WS_WANT_READ) { + /* What commits is the open reaching the peer, not the return: + * a highwater callback failing after the flush is not a send + * that never left. */ + if (!SendPacketDelivered(ssh, flushes, ret)) { ChannelDelete(newChannel, ssh->ctx->heap); } else { diff --git a/src/internal.c b/src/internal.c index d12d8f38..b2153e32 100644 --- a/src/internal.c +++ b/src/internal.c @@ -19073,19 +19073,8 @@ int SendIgnore(WOLFSSH* ssh, const unsigned char* data, word32 dataSz) return ret; } -/* Will the packet just framed reach the peer? A completed flush says so; the - * return does not, since the highwater callback runs after the last byte goes - * out and the rekey it starts fails with the same codes a lost send does. - * Comparing the flush count across the send tells those apart. - * - * Short of a flush, WS_WANT_WRITE is the one outcome that keeps the packet - * framed for the next one; an interrupted send is retried inside - * wolfSSH_SendPacket() rather than reported. Anything else counts as not sent, - * which at worst leaves the peer holding a request this side did not register; - * guessing the other way would desync the reply queue for the life of the - * session. Call before anything else runs, since a later send flushes this - * packet and would read as this one's. */ -static INLINE int SendPacketDelivered(WOLFSSH* ssh, word32 flushes, int ret) +/* Contract in internal.h. */ +int SendPacketDelivered(WOLFSSH* ssh, word32 flushes, int ret) { return ssh->txFlushCount != flushes || ret == WS_WANT_WRITE; } diff --git a/tests/regress.c b/tests/regress.c index 233d3b2b..c8c3016d 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4557,6 +4557,56 @@ static void TestAgentChannelOpenSendFailureCleansUp(void) FreeChannelOpenHarness(&harness); } +/* Fails the session's first highwater check, and counts its calls. */ +static int AgentOpenHighwaterCb(byte side, void* ctx) +{ + int* calls = (int*)ctx; + + WOLFSSH_UNUSED(side); + + (*calls)++; + return WS_FATAL_ERROR; +} + +/* The highwater callback fails after the open is out, so its error arrives as + * the send's return. Rolling back on that deletes a channel the peer has, and + * its confirmation then names nothing. */ +static void TestAgentChannelOpenHighwaterErrorKeepsChannel(void) +{ + ChannelOpenHarness harness; + int calls = 0; + word32 outSz; + + InitChannelOpenHarness(&harness, NULL, 0); + harness.ssh->useAgent = 1; + + wolfSSH_SetHighwaterCb(harness.ctx, 1, AgentOpenHighwaterCb); + wolfSSH_SetHighwaterCtx(harness.ssh, &calls); + harness.ssh->highwaterMark = 1; + harness.ssh->txCount = 1; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_FATAL_ERROR); + AssertIntEQ(calls, 1); + AssertIntEQ(harness.ssh->error, WS_FATAL_ERROR); + + /* The open went out, so the channel and the agent stand. */ + AssertNotNull(harness.ssh->agent); + AssertIntEQ(harness.ssh->channelListSz, 1); + AssertTrue(harness.io.outSz > 0); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_CHANNEL_OPEN); + + /* The mark has fired, so the next poll is the idempotent one. */ + outSz = harness.io.outSz; + + AssertIntEQ(wolfSSH_AGENT_ChannelOpen(harness.ssh), WS_SUCCESS); + AssertIntEQ(calls, 1); + AssertIntEQ(harness.ssh->channelListSz, 1); + AssertIntEQ(harness.io.outSz, outSz); + + FreeChannelOpenHarness(&harness); +} + #ifndef NO_WOLFSSH_CLIENT /* Server-side call. A client has an ssh->agent of its own, so answering the * poll from it would report a channel that was never opened. */ @@ -13570,6 +13620,7 @@ int main(int argc, char** argv) TestAgentChannelOpenAfterDisconnect(); TestAgentChannelOpenQueuedThenDisconnect(); TestAgentChannelOpenSendFailureCleansUp(); + TestAgentChannelOpenHighwaterErrorKeepsChannel(); #ifndef NO_WOLFSSH_CLIENT TestAgentChannelOpenOnClientRefused(); #endif diff --git a/wolfssh/agent.h b/wolfssh/agent.h index f2bad7fb..0139cacd 100644 --- a/wolfssh/agent.h +++ b/wolfssh/agent.h @@ -190,7 +190,9 @@ WOLFSSH_API int wolfSSH_AGENT_enable(WOLFSSH* ssh, byte isEnabled); * WS_FATAL_ERROR with WS_DISCONNECT in ssh->error once the session is over, * WS_SSH_NULL_E, WS_MEMORY_E, or whatever the send reports. WS_SUCCESS says * the open went out, not that the peer took it; a refusal reaches the - * channel-open-fail callback. + * channel-open-fail callback. An error raised after the open is on the wire, + * a failing highwater callback, leaves the channel open and the next poll + * answers WS_SUCCESS. * Only that and the send record in ssh->error, so a poll ahead of the peer's * request leaves the session fit for wolfSSH_accept(). */ WOLFSSH_API int wolfSSH_AGENT_ChannelOpen(WOLFSSH* ssh); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index f4cd78c6..be5905af 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1689,6 +1689,18 @@ WOLFSSH_LOCAL int DoReceive(WOLFSSH* ssh); WOLFSSH_LOCAL int DoProtoId(WOLFSSH* ssh); WOLFSSH_LOCAL int wolfSSH_SendPacket(WOLFSSH* ssh); WOLFSSH_LOCAL int wolfSSH_OutputPending(WOLFSSH* ssh); +/* Will the packet just framed reach the peer? A completed flush says so; the + * return does not, since the highwater callback runs after the last byte is + * out and fails with the same codes a lost send does. Take flushes from + * ssh->txFlushCount before the send, and call this before anything else runs: + * a later send flushes this packet and would read as this one's. + * + * Short of a flush, only WS_WANT_WRITE keeps the packet framed for the next + * one; an interrupt is retried inside wolfSSH_SendPacket(), not reported. + * Anything else counts as not sent, which at worst leaves the peer holding a + * request this side did not register; the other guess desyncs the reply queue + * for the life of the session. */ +WOLFSSH_LOCAL int SendPacketDelivered(WOLFSSH* ssh, word32 flushes, int ret); WOLFSSH_LOCAL int SendProtoId(WOLFSSH* ssh); WOLFSSH_LOCAL int ValidateProtoId(const char* protoIdStr, word32 len); WOLFSSH_LOCAL int SendKexInit(WOLFSSH* ssh);