agent: commit the open on a completed flush

wolfSSH_AGENT_ChannelOpen() settles its channel and agent on whether
the open reached the peer, which SendPacketDelivered() answers from the
flush count. The highwater callback runs after the last byte is out, so
its failure arrives as this send's return, and the rollback read that
as a send that never left.

- give SendPacketDelivered() external linkage, contract in internal.h
- note in agent.h that an error raised once the open is on the wire
  leaves the channel open
- test a failing highwater callback: the channel and the agent stand,
  and the poll after it is the idempotent one

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 {

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;
 }

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

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);

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);