Commit forward state before the post-send callback

The forward a "tcpip-forward" or "cancel-tcpip-forward" establishes was
committed after SendGlobalRequestFwd() returned, which is after the
post-send highwater callback had run. A request that callback sends goes
out behind this one but committed ahead of it, so the earlier request
had the last word and the client ended up on the opposite side of the
forward from the peer. A first setup whose callback cancels it left the
forward registered with no listener on the peer, and a cancel whose
callback re-establishes the forward unlinked it, refusing every open for
a listener the peer holds.

- Split the post-send highwater check off wolfSSH_SendPacket() as
  SendPacketFlush(), for a sender with state to commit first.
- SendGlobalRequestFwd() takes the pending forward and settles it inside
  the send window, then runs the check.
- Commit order is send order now, so the last request sent governs,
  whichever call made it.
- FwdPendingCommit() still re-resolves the entry: the IO send callback
  can reenter mid-flush, which no ordering fixes.
- Tests cover a reentrant cancel of a first setup, a reentrant setup
  during a cancel, and an inbound forwarded-tcpip open pumped from the
  callback.

Author: ejohnstown

src/internal.c

@@ -4919,10 +4919,15 @@ static int GetInputLine(WOLFSSH* ssh, byte** pEol)
 }
 
 
-/* returns WS_SUCCESS on success */
-int wolfSSH_SendPacket(WOLFSSH* ssh)
+/* Push everything framed at the peer, stopping short of the post-send
+ * highwater check. A sender with state to commit runs that check itself, after
+ * committing: the callback it fires can reenter the library and send a request
+ * of its own, which goes out behind this one and has to commit behind it too.
+ *
+ * returns WS_SUCCESS on success */
+static int SendPacketFlush(WOLFSSH* ssh)
 {
-    WLOG(WS_LOG_DEBUG, "Entering wolfSSH_SendPacket()");
+    WLOG(WS_LOG_DEBUG, "Entering SendPacketFlush()");
 
     if (ssh->ctx->ioSendCb == NULL) {
         WLOG(WS_LOG_DEBUG, "Your IO Send callback is null, please set");
@@ -4999,7 +5004,23 @@ int wolfSSH_SendPacket(WOLFSSH* ssh)
 
     WLOG(WS_LOG_DEBUG, "SB: Shrinking output buffer");
     ShrinkBuffer(&ssh->outputBuffer, 0);
-    return HighwaterCheck(ssh, WOLFSSH_HWSIDE_TRANSMIT);
+    return WS_SUCCESS;
+}
+
+
+/* returns WS_SUCCESS on success */
+int wolfSSH_SendPacket(WOLFSSH* ssh)
+{
+    int ret;
+
+    ret = SendPacketFlush(ssh);
+
+    /* Only a complete flush reaches the check, as the peer has the whole
+     * packet by then. */
+    if (ret == WS_SUCCESS)
+        ret = HighwaterCheck(ssh, WOLFSSH_HWSIDE_TRANSMIT);
+
+    return ret;
 }
 
 
@@ -17678,23 +17699,24 @@ int SendGlobalRequest(WOLFSSH* ssh,
 #ifdef WOLFSSH_FWD
 /* Send a "tcpip-forward" or "cancel-tcpip-forward" global request. The bind
  * address and port follow the want-reply boolean, an ordering the generic
- * SendGlobalRequest() framing cannot express. RFC 4254 7.1. */
+ * SendGlobalRequest() framing cannot express. RFC 4254 7.1.
+ *
+ * What FwdRemotePrepare() built for the request is settled here rather than by
+ * the caller, since it has to happen inside the send window. */
 int SendGlobalRequestFwd(WOLFSSH* ssh,
         const char* bindAddr, word32 bindPort, int isCancel, int wantReply,
-        int* sent)
+        WOLFSSH_FWD_PENDING* pend)
 {
     byte* output;
     word32 idx = 0;
     word32 reqNameSz;
     word32 bindAddrSz;
     const char* reqName;
+    int sent = 0;
     int ret = WS_SUCCESS;
 
     WLOG(WS_LOG_DEBUG, "Entering SendGlobalRequestFwd()");
 
-    if (sent != NULL)
-        *sent = 0;
-
     if (ssh == NULL || bindAddr == NULL)
         ret = WS_BAD_ARGUMENT;
 
@@ -17732,12 +17754,24 @@ int SendGlobalRequestFwd(WOLFSSH* ssh,
     if (ret == WS_SUCCESS) {
         word32 flushes = ssh->txFlushCount;
 
-        ret = wolfSSH_SendPacket(ssh);
-
-        if (sent != NULL)
-            *sent = SendPacketDelivered(ssh, flushes, ret);
+        ret = SendPacketFlush(ssh);
+        sent = SendPacketDelivered(ssh, flushes, ret);
     }
 
+    /* Whether the peer will bind the listener, not whether this call
+     * succeeded: a request still framed and waiting to flush reaches it. Only
+     * what never left unwinds. */
+    if (sent)
+        FwdPendingCommit(ssh, pend);
+    else
+        FwdPendingDiscard(ssh, pend);
+
+    /* Held back until the commit is done. The callback can reenter and send a
+     * request of its own, which goes out behind this one, and the last request
+     * sent is the one that governs. */
+    if (ret == WS_SUCCESS)
+        ret = HighwaterCheck(ssh, WOLFSSH_HWSIDE_TRANSMIT);
+
     WLOG(WS_LOG_DEBUG, "Leaving SendGlobalRequestFwd(), ret = %d", ret);
 
     return ret;

src/ssh.c

@@ -3902,21 +3902,11 @@ int wolfSSH_FwdRemoteSetup(WOLFSSH* ssh, const char* bindAddr,
     if (ret == WS_SUCCESS)
         ret = FwdRemotePrepare(ssh, bindAddr, bindPort, wantReply, 0, &pend);
 
-    if (ret == WS_SUCCESS) {
-        int sent = 0;
-
+    /* The send settles pend: what reached the peer registers, even when the
+     * post-send highwater callback reports an error afterwards. */
+    if (ret == WS_SUCCESS)
         ret = SendGlobalRequestFwd(ssh, bindAddr, bindPort, 0, wantReply,
-                &sent);
-
-        /* Whether the peer will bind the listener, not whether this call
-         * succeeded: a request still framed and waiting to flush reaches it,
-         * and so does one the post-send highwater callback reports an error
-         * for. Only what never left unwinds. */
-        if (sent)
-            FwdPendingCommit(ssh, &pend);
-        else
-            FwdPendingDiscard(ssh, &pend);
-    }
+                &pend);
 
     WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_FwdRemoteSetup(), ret = %d", ret);
     return ret;
@@ -3956,17 +3946,9 @@ int wolfSSH_FwdRemoteCancel(WOLFSSH* ssh, const char* bindAddr,
     if (ret == WS_SUCCESS)
         ret = FwdRemotePrepare(ssh, bindAddr, bindPort, wantReply, 1, &pend);
 
-    if (ret == WS_SUCCESS) {
-        int sent = 0;
-
+    if (ret == WS_SUCCESS)
         ret = SendGlobalRequestFwd(ssh, bindAddr, bindPort, 1, wantReply,
-                &sent);
-
-        if (sent)
-            FwdPendingCommit(ssh, &pend);
-        else
-            FwdPendingDiscard(ssh, &pend);
-    }
+                &pend);
 
     WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_FwdRemoteCancel(), ret = %d", ret);
     return ret;

tests/regress.c

@@ -4570,6 +4570,123 @@ static void TestForwardedTcpipCancelAnsweredDuringResetupKeepsForward(void)
 /* A peer that canonicalises the bind it echoes back has every open refused
  * under the default, so the port it was asked for can be made the whole
  * test. */
+static int CancelFirstSetupHighwaterCb(byte side, void* ctx)
+{
+    WOLFSSH* ssh = (WOLFSSH*)ctx;
+
+    WOLFSSH_UNUSED(side);
+
+    if (ssh != NULL)
+        wolfSSH_FwdRemoteCancel(ssh, "127.0.0.1", 8080, 0);
+
+    return WS_SUCCESS;
+}
+
+/* The callback cancels the very forward the request in flight is establishing,
+ * and with no earlier registration to find it has nothing to work from but
+ * what this request left. The cancel went out behind the setup, so the peer
+ * holds no listener and neither may this side. */
+static void TestForwardedTcpipReentrantCancelOfFirstSetup(void)
+{
+    ChannelOpenHarness harness;
+
+    InitFwdRemoteHarness(&harness);
+
+    wolfSSH_SetHighwaterCb(harness.ctx, 1, CancelFirstSetupHighwaterCb);
+    wolfSSH_SetHighwaterCtx(harness.ssh, harness.ssh);
+    /* Cross the mark on the request's own send. */
+    harness.ssh->highwaterMark = 1;
+    harness.ssh->txCount = 1;
+
+    AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 0),
+            WS_SUCCESS);
+
+    AssertIntEQ(FwdRemoteCount(harness.ssh), 0);
+
+    harness.io.outSz = 0;
+    AssertForwardedOpenRefused(&harness, "127.0.0.1", 8080);
+
+    FreeChannelOpenHarness(&harness);
+}
+
+static int SetupDuringCancelHighwaterCb(byte side, void* ctx)
+{
+    WOLFSSH* ssh = (WOLFSSH*)ctx;
+
+    WOLFSSH_UNUSED(side);
+
+    if (ssh != NULL)
+        wolfSSH_FwdRemoteSetup(ssh, "127.0.0.1", 8080, 0);
+
+    return WS_SUCCESS;
+}
+
+/* The same window the other way around: the callback re-establishes the
+ * forward the cancel in flight is taking down. The setup went out behind the
+ * cancel, so the peer binds a listener and this side keeps matching for it. */
+static void TestForwardedTcpipReentrantSetupDuringCancel(void)
+{
+    ChannelOpenHarness harness;
+
+    InitFwdRemoteHarness(&harness);
+
+    AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 0),
+            WS_SUCCESS);
+
+    wolfSSH_SetHighwaterCb(harness.ctx, 1, SetupDuringCancelHighwaterCb);
+    wolfSSH_SetHighwaterCtx(harness.ssh, harness.ssh);
+    harness.ssh->highwaterMark = 1;
+    harness.ssh->txCount = 1;
+
+    AssertIntEQ(wolfSSH_FwdRemoteCancel(harness.ssh, "127.0.0.1", 8080, 0),
+            WS_SUCCESS);
+
+    AssertIntEQ(FwdRemoteCount(harness.ssh), 1);
+
+    harness.io.outSz = 0;
+    AssertForwardedOpenRefused(&harness, "10.0.0.1", 9999);
+    AssertForwardedOpenAccepted(&harness, "127.0.0.1", 8080, 1);
+
+    FreeChannelOpenHarness(&harness);
+}
+
+static int InboundOpenDuringSendHighwaterCb(byte side, void* ctx)
+{
+    ChannelOpenHarness* harness = (ChannelOpenHarness*)ctx;
+
+    WOLFSSH_UNUSED(side);
+
+    /* The request is on the wire, so matching has to be live already: this is
+     * the first setup, and until it registers nothing is tracked and every
+     * open goes unchecked. */
+    AssertIntEQ(harness->ssh->fwdRemoteTracked, 1);
+    AssertForwardedOpenRefused(harness, "10.0.0.1", 9999);
+
+    return WS_SUCCESS;
+}
+
+/* A callback that pumps the session sees the forwards the request in flight
+ * established, not the ones it found on the way in. */
+static void TestForwardedTcpipInboundOpenDuringSend(void)
+{
+    ChannelOpenHarness harness;
+
+    InitFwdRemoteHarness(&harness);
+
+    wolfSSH_SetHighwaterCb(harness.ctx, 1, InboundOpenDuringSendHighwaterCb);
+    wolfSSH_SetHighwaterCtx(harness.ssh, &harness);
+    harness.ssh->highwaterMark = 1;
+    harness.ssh->txCount = 1;
+
+    AssertIntEQ(wolfSSH_FwdRemoteSetup(harness.ssh, "127.0.0.1", 8080, 0),
+            WS_SUCCESS);
+
+    harness.io.outSz = 0;
+    AssertForwardedOpenAccepted(&harness, "127.0.0.1", 8080, 1);
+
+    FreeChannelOpenHarness(&harness);
+}
+
 static void TestFwdRemoteMatchPortIgnoresBindAddr(void)
 {
     RunForwardedTcpipMatchModeTest(WOLFSSH_FWD_MATCH_STRICT, "localhost", 8080,
@@ -10348,6 +10465,9 @@ int main(int argc, char** argv)
     TestForwardedTcpipRequestAfterReplyDuringSend();
     TestForwardedTcpipCancelAnsweredDuringResetupKeepsForward();
     TestForwardedTcpipReentrantCancelDuringSend();
+    TestForwardedTcpipReentrantCancelOfFirstSetup();
+    TestForwardedTcpipReentrantSetupDuringCancel();
+    TestForwardedTcpipInboundOpenDuringSend();
     TestFwdRemoteMatchPortIgnoresBindAddr();
     TestFwdRemoteMatchOffAcceptsUnregistered();
     TestFwdRemoteMatchRejectsBadSetting();

wolfssh/internal.h

@@ -1638,9 +1638,13 @@ WOLFSSH_LOCAL int SendGlobalRequestFwdSuccess(WOLFSSH * ssh, int success,
 WOLFSSH_LOCAL int SendGlobalRequest(WOLFSSH * ssh,
         const unsigned char * data, word32 dataSz, int reply, int* sent);
 #ifdef WOLFSSH_FWD
+/* Sends the request and settles pend with it: committed once the request is on
+ * its way to the peer, discarded when it never left. Both happen before the
+ * post-send highwater callback runs, so a request that callback sends commits
+ * behind this one. */
 WOLFSSH_LOCAL int SendGlobalRequestFwd(WOLFSSH* ssh,
         const char* bindAddr, word32 bindPort, int isCancel, int wantReply,
-        int* sent);
+        WOLFSSH_FWD_PENDING* pend);
 /* On success pend holds what to commit once the request reaches the wire; on
  * error it is zeroed, so there is nothing to commit or give back. */
 WOLFSSH_LOCAL int FwdRemotePrepare(WOLFSSH* ssh, const char* bindAddr,

wolfssh/ssh.h

@@ -291,7 +291,9 @@ DEPRECATED WOLFSSH_API int wolfSSH_ChannelGetFwdFd(
  * Several requests can name one bind at once, and the last one sent governs.
  * Registering again while a cancel is outstanding brings the forward back as
  * the request goes out, and no answer to that older cancel takes it away
- * again, whatever order the peer answers in.
+ * again, whatever order the peer answers in. A request one of these calls
+ * makes from a callback it fires is no different: it goes out behind this
+ * one, so it is the one that governs.
  *
  * WS_WANT_WRITE means the request is framed and goes out on the next flush,
  * with the forward registered. So does an error reported after the request