diff --git a/src/internal.c b/src/internal.c index 6ac443ae..f2924865 100644 --- a/src/internal.c +++ b/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; } @@ -17737,23 +17758,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; @@ -17791,12 +17813,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; diff --git a/src/ssh.c b/src/ssh.c index f117a2ef..2e5019f2 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -3991,21 +3991,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; @@ -4045,17 +4035,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; diff --git a/tests/regress.c b/tests/regress.c index 4703f9e5..a0f9fa8b 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4718,6 +4718,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, @@ -10499,6 +10616,9 @@ int main(int argc, char** argv) TestForwardedTcpipRequestAfterReplyDuringSend(); TestForwardedTcpipCancelAnsweredDuringResetupKeepsForward(); TestForwardedTcpipReentrantCancelDuringSend(); + TestForwardedTcpipReentrantCancelOfFirstSetup(); + TestForwardedTcpipReentrantSetupDuringCancel(); + TestForwardedTcpipInboundOpenDuringSend(); TestFwdRemoteMatchPortIgnoresBindAddr(); TestFwdRemoteMatchOffAcceptsUnregistered(); TestFwdRemoteMatchRejectsBadSetting(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 065fcb5d..2201d4ca 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1637,9 +1637,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, diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 91e1b8d7..ec173747 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -319,7 +319,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