diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 00320b7c8..9bda32704 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -508,17 +508,28 @@ static int wolfSSH_FwdDefaultActions(WS_FwdCbAction action, void* vCtx, appCtx->state = APP_STATE_CONNECT; } else if (action == WOLFSSH_FWD_LOCAL_CLEANUP) { - WCLOSESOCKET(appCtx->appFd); - appCtx->appFd = -1; - if (fwdCbCtx->hostName) { - WFREE(fwdCbCtx->hostName, NULL, 0); - fwdCbCtx->hostName = NULL; - } - if (fwdCbCtx->originName) { - WFREE(fwdCbCtx->originName, NULL, 0); - fwdCbCtx->originName = NULL; + /* The channel id rides in the port parameter. A channel can outlive + * its turn in the slot, so only the holder may tear it down. */ + if (port == appCtx->channelId) { + /* This runs now, so the socket may already be gone: the open can + * fail after the setup, before anything connected. */ + if (appCtx->appFd != (WS_SOCKET_T)-1) { + WCLOSESOCKET(appCtx->appFd); + appCtx->appFd = -1; + } + if (fwdCbCtx->hostName) { + WFREE(fwdCbCtx->hostName, NULL, 0); + fwdCbCtx->hostName = NULL; + } + if (fwdCbCtx->originName) { + WFREE(fwdCbCtx->originName, NULL, 0); + fwdCbCtx->originName = NULL; + } + /* A refused connect leaves this set; retire it with the + * channel. */ + fwdCbCtx->isDirect = 0; + appCtx->state = APP_STATE_INIT; } - appCtx->state = APP_STATE_INIT; } else if (action == WOLFSSH_FWD_REMOTE_SETUP) { struct sockaddr_in addr; @@ -1181,21 +1192,35 @@ static int ssh_worker(thread_ctx_t* threadCtx) } else if (rc == WS_CHANNEL_CLOSED) { #ifdef WOLFSSH_FWD - if (threadCtx->fwdCtx.state == APP_STATE_CONNECTED && - lastChannel == threadCtx->fwdCtx.channelId) { - /* Read zero-returned. Socket is closed. Go back - to listening. */ - if (fwdFd != -1) { - WCLOSESOCKET(fwdFd); + /* wolfSSH_worker() names the channel only for the + * data and EOF statuses; DoChannelClose() recorded + * the id it retired. */ + wolfSSH_GetLastRxId(ssh, &lastChannel); + if (lastChannel == threadCtx->fwdCtx.channelId) { + if (threadCtx->fwdCtx.appFd == -1) { + /* The LOCAL_CLEANUP handler ran ahead of + * this and closed the socket; only this + * copy of the descriptor is stale. */ fwdFd = -1; - threadCtx->fwdCtx.appFd = -1; } - if (threadCtx->fwdCbCtx.originName != NULL) { - WFREE(threadCtx->fwdCbCtx.originName, - NULL, 0); - threadCtx->fwdCbCtx.originName = NULL; + else if (threadCtx->fwdCtx.state + == APP_STATE_CONNECTED) { + /* A locally opened forward is armed by no + * LOCAL_SETUP and so draws no cleanup. Its + * teardown is still ours: go back to + * listening. */ + if (fwdFd != -1) { + WCLOSESOCKET(fwdFd); + fwdFd = -1; + threadCtx->fwdCtx.appFd = -1; + } + if (threadCtx->fwdCbCtx.originName != NULL) { + WFREE(threadCtx->fwdCbCtx.originName, + NULL, 0); + threadCtx->fwdCbCtx.originName = NULL; + } + threadCtx->fwdCtx.state = APP_STATE_LISTEN; } - threadCtx->fwdCtx.state = APP_STATE_LISTEN; } #endif continue; diff --git a/examples/portfwd/portfwd.c b/examples/portfwd/portfwd.c index e406368dc..f2a95e75f 100644 --- a/examples/portfwd/portfwd.c +++ b/examples/portfwd/portfwd.c @@ -249,6 +249,7 @@ typedef struct PortfwdState { SOCKET_T appFd; /* socket to the local target, -1 when idle */ word32 channelId; /* id of the inbound forwarded-tcpip channel */ int pending; /* a new channel is waiting to be wired up */ + int cleanupRxd; /* LOCAL_CLEANUP closed appFd for us */ int replied; /* peer answered the tcpip-forward request */ int refused; /* ...and the answer was a refusal */ int badPort; /* ...or named a port outside 1..65535 */ @@ -319,15 +320,17 @@ static int portfwdRemoteFwdCb(WS_FwdCbAction action, void* ctx, st->pending = 1; break; case WOLFSSH_FWD_LOCAL_CLEANUP: - /* The library does not currently emit this action, so this branch - * never runs. The target socket is closed when portfwd_worker() - * leaves its loop. Kept so the handler is right if that changes. */ + /* Paired with the LOCAL_SETUP that opened the target socket. + * portfwd_worker() keeps its own copy of the descriptor, so tell + * it not to close what has already been closed. The closing + * channel's id arrives in the port argument. */ (void)address; (void)port; if (st->appFd != (SOCKET_T)-1) { WCLOSESOCKET(st->appFd); st->appFd = (SOCKET_T)-1; } + st->cleanupRxd = 1; break; case WOLFSSH_FWD_REMOTE_SETUP: case WOLFSSH_FWD_REMOTE_CLEANUP: @@ -746,6 +749,9 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) WS_CHANNEL_ID_SELF); if (fwdState.appFd != (SOCKET_T)-1 && newChannel != NULL) { appFd = fwdState.appFd; + /* The latch describes this descriptor now, not one an + * earlier failed open already cleaned up. */ + fwdState.cleanupRxd = 0; fwdChannel = newChannel; fwdChannelId = fwdState.channelId; FD_SET(appFd, &templateFds); @@ -885,7 +891,17 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args) WCLOSESOCKET(sshFd); if (listenFd != (SOCKET_T)-1) WCLOSESOCKET(listenFd); - WCLOSESOCKET(appFd); + /* Skip a descriptor the cleanup callback already closed; closing it + * twice can take down whatever has been handed the number since. */ + if (fwdState.cleanupRxd) + appFd = (SOCKET_T)-1; + if (appFd != (SOCKET_T)-1) { + WCLOSESOCKET(appFd); + /* The loop can leave with the channel still open, and freeing the + * session below runs the cleanup handler on this same descriptor. */ + if (fwdState.appFd == appFd) + fwdState.appFd = (SOCKET_T)-1; + } wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); #ifdef WOLFSSH_SMALL_STACK diff --git a/src/internal.c b/src/internal.c index 81e41ff9a..182c43df4 100644 --- a/src/internal.c +++ b/src/internal.c @@ -3840,12 +3840,45 @@ WOLFSSH_CHANNEL* ChannelNew(WOLFSSH* ssh, byte channelType, } +#ifdef WOLFSSH_FWD +/* Counterpart of the WOLFSSH_FWD_LOCAL_SETUP sent when a forwarding channel + * was opened, so the application can release what it set up there. Gated on + * that setup having succeeded: a locally opened forward draws no setup, and + * one that reported failure set nothing up. Cleaning up after either would + * free what the application does not own. Runs from ChannelDelete() so every + * way a channel goes, a peer close, a refused open, wolfSSH_ChannelFree(), + * or the session being freed, reports it once. + * The channel's id rides in the port parameter, the way CHANNEL_ID passes + * it. */ +static void NotifyFwdLocalCleanup(WOLFSSH_CHANNEL* channel) +{ + WOLFSSH* ssh; + int ret; + + if (channel == NULL || !channel->fwdSetupTxd) + return; + ssh = channel->ssh; + if (ssh == NULL || ssh->ctx->fwdCb == NULL) + return; + + channel->fwdSetupTxd = 0; + ret = ssh->ctx->fwdCb(WOLFSSH_FWD_LOCAL_CLEANUP, ssh->fwdCbCtx, + NULL, channel->channel); + if (ret != WS_SUCCESS) { + WLOG(WS_LOG_WARN, "Forward cleanup failed for channel %u, ret = %d", + channel->channel, ret); + } +} +#endif /* WOLFSSH_FWD */ + + void ChannelDelete(WOLFSSH_CHANNEL* channel, void* heap) { WOLFSSH_UNUSED(heap); if (channel) { #ifdef WOLFSSH_FWD + NotifyFwdLocalCleanup(channel); if (channel->host) WFREE(channel->host, heap, DYNTYPE_STRING); if (channel->origin) @@ -12053,6 +12086,9 @@ static int DoChannelOpen(WOLFSSH* ssh, ret = ssh->ctx->fwdCb(WOLFSSH_FWD_LOCAL_SETUP, ssh->fwdCbCtx, host, hostPort); if (ret == WS_SUCCESS) { + /* The application now owns whatever the setup made, + * so it is owed the matching cleanup. */ + newChannel->fwdSetupTxd = 1; ret = ssh->ctx->fwdCb(WOLFSSH_FWD_CHANNEL_ID, ssh->fwdCbCtx, NULL, newChannel->channel); } diff --git a/tests/regress.c b/tests/regress.c index 9f6cd451a..c87c888d7 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -1966,6 +1966,51 @@ static int RejectDirectTcpipSetup(WS_FwdCbAction action, void* ctx, return WS_SUCCESS; } +#define REGRESS_FWD_ACTION_MAX 8 +static WS_FwdCbAction fwdActions[REGRESS_FWD_ACTION_MAX]; +static word32 fwdActionCount; + +/* Records the actions the library asked for, so a test can assert the + * sequence a forwarding channel produces. */ +static int RecordingFwdCb(WS_FwdCbAction action, void* ctx, + const char* host, word32 port) +{ + (void)ctx; + (void)host; + (void)port; + + if (fwdActionCount < REGRESS_FWD_ACTION_MAX) + fwdActions[fwdActionCount] = action; + fwdActionCount++; + + return WS_SUCCESS; +} + +/* As RecordingFwdCb, but refuses the channel-id handoff so the open fails + * after a successful setup. */ +static int RecordingRejectChannelIdFwdCb(WS_FwdCbAction action, void* ctx, + const char* host, word32 port) +{ + int ret = RecordingFwdCb(action, ctx, host, port); + + if (action == WOLFSSH_FWD_CHANNEL_ID) + return WS_FWD_NOT_AVAILABLE; + + return ret; +} + +/* As RecordingFwdCb, but refuses the setup itself. */ +static int RecordingRejectSetupFwdCb(WS_FwdCbAction action, void* ctx, + const char* host, word32 port) +{ + int ret = RecordingFwdCb(action, ctx, host, port); + + if (action == WOLFSSH_FWD_LOCAL_SETUP) + return WS_FWD_SETUP_E; + + return ret; +} + /* Counts every action the library asks for. File scope rather than reached * through the callback ctx, so a zero reading means the hook did not run and * cannot instead mean the ctx stopped being delivered. */ @@ -3799,9 +3844,231 @@ static void TestDirectTcpipFwdCbRejectsChannelId(void) * unlike the rejections that set the reason themselves. */ AssertIntEQ(ParseChannelOpenFailReason(harness.io.out, harness.io.outSz), OPEN_ADMINISTRATIVELY_PROHIBITED); - AssertIntEQ(fwdCbCallCount, 2); + /* Three, not two: the setup that succeeded is owed its cleanup even + * though the open went on to fail. */ + AssertIntEQ(fwdCbCallCount, 3); + + FreeChannelOpenHarness(&harness); +} + +/* A forwarding channel that opened with a LOCAL_SETUP has to report a + * LOCAL_CLEANUP when it closes, or an application has no hook to release what + * it set up. */ +static void TestDirectTcpipCloseSendsLocalCleanup(void) +{ + ChannelOpenHarness harness; + byte extra[128]; + byte in[192]; + byte in2[64]; + word32 extraSz; + word32 inSz; + word32 in2Sz; + word32 selfChannelId; + int ret; + + fwdActionCount = 0; + + extraSz = BuildDirectTcpipExtra("127.0.0.1", 8080, "127.0.0.1", 2222, + extra, sizeof(extra)); + inSz = BuildChannelOpenPacket("direct-tcpip", 9, 0x4000, 0x8000, + extra, extraSz, in, sizeof(in)); + + InitChannelOpenHarness(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, RecordingFwdCb, NULL), + WS_SUCCESS); + + ret = DoReceive(harness.ssh); + AssertIntEQ(ret, WS_SUCCESS); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_CHANNEL_OPEN_CONF); + AssertIntEQ(harness.ssh->channelListSz, 1); + AssertIntEQ(fwdActionCount, 2); + AssertIntEQ(fwdActions[0], WOLFSSH_FWD_LOCAL_SETUP); + AssertIntEQ(fwdActions[1], WOLFSSH_FWD_CHANNEL_ID); + + selfChannelId = harness.ssh->channelList->channel; + in2Sz = BuildChannelClosePacket(selfChannelId, in2, sizeof(in2)); + RepointHarnessInput(&harness, in2, in2Sz); + + ret = DoReceive(harness.ssh); + AssertIntEQ(ret, WS_CHANNEL_CLOSED); + AssertIntEQ(harness.ssh->channelListSz, 0); + AssertIntEQ(fwdActionCount, 3); + AssertIntEQ(fwdActions[2], WOLFSSH_FWD_LOCAL_CLEANUP); + + FreeChannelOpenHarness(&harness); +} + +/* Closes a channel this side created itself, which never saw a + * LOCAL_SETUP, and reports how many forwarding actions that produced. */ +static word32 CloseSelfOpenedChannel(byte channelType) +{ + ChannelOpenHarness harness; + WOLFSSH_CHANNEL* channel; + byte in[64]; + word32 inSz; + word32 selfChannelId; + + fwdActionCount = 0; + + InitChannelOpenHarness(&harness, NULL, 0); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, RecordingFwdCb, NULL), + WS_SUCCESS); + + channel = ChannelNew(harness.ssh, channelType, 1024, 1024); + AssertNotNull(channel); + AssertIntEQ(ChannelUpdatePeer(channel, 5, 1024, 1024), WS_SUCCESS); + AssertIntEQ(ChannelAppend(harness.ssh, channel), WS_SUCCESS); + channel->openConfirmed = 1; + selfChannelId = channel->channel; + + inSz = BuildChannelClosePacket(selfChannelId, in, sizeof(in)); + RepointHarnessInput(&harness, in, inSz); + + AssertIntEQ(DoReceive(harness.ssh), WS_CHANNEL_CLOSED); + AssertIntEQ(harness.ssh->channelListSz, 0); + + FreeChannelOpenHarness(&harness); + + return fwdActionCount; +} + +/* wolfSSH_ChannelFwdNewLocal() and wolfSSH_ChannelFwdNewRemote() build a + * forwarding channel without a LOCAL_SETUP, so its close owes no cleanup: + * the application never armed one and would be freeing what it does not + * own. This is why the emission is gated on the channel, not its type. */ +static void TestLocalForwardCloseSendsNoCleanup(void) +{ + AssertIntEQ(CloseSelfOpenedChannel(ID_CHANTYPE_TCPIP_DIRECT), 0); +} + +/* And a session channel produces no forwarding action at all. */ +static void TestSessionCloseSendsNoFwdAction(void) +{ + AssertIntEQ(CloseSelfOpenedChannel(ID_CHANTYPE_SESSION), 0); +} + +/* A setup that succeeded is owed its cleanup even when the open goes on to + * fail, or the application keeps a socket for a channel that never existed. + * The channel-id handoff rejecting is the reachable way in. */ +static void TestFailedOpenAfterSetupSendsCleanup(void) +{ + ChannelOpenHarness harness; + byte extra[128]; + byte in[192]; + word32 extraSz; + word32 inSz; + int ret; + + fwdActionCount = 0; + + extraSz = BuildDirectTcpipExtra("127.0.0.1", 8080, "127.0.0.1", 2222, + extra, sizeof(extra)); + inSz = BuildChannelOpenPacket("direct-tcpip", 9, 0x4000, 0x8000, + extra, extraSz, in, sizeof(in)); + + InitChannelOpenHarness(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, + RecordingRejectChannelIdFwdCb, NULL), WS_SUCCESS); + + ret = DoReceive(harness.ssh); + AssertChannelOpenFailResponse(&harness, ret); + AssertIntEQ(harness.ssh->channelListSz, 0); + AssertIntEQ(fwdActionCount, 3); + AssertIntEQ(fwdActions[0], WOLFSSH_FWD_LOCAL_SETUP); + AssertIntEQ(fwdActions[1], WOLFSSH_FWD_CHANNEL_ID); + AssertIntEQ(fwdActions[2], WOLFSSH_FWD_LOCAL_CLEANUP); + + FreeChannelOpenHarness(&harness); +} + +/* A setup that reported failure set nothing up, so it is owed no cleanup; + * one would have the application release what it never acquired. */ +static void TestRejectedSetupSendsNoCleanup(void) +{ + ChannelOpenHarness harness; + byte extra[128]; + byte in[192]; + word32 extraSz; + word32 inSz; + int ret; + + fwdActionCount = 0; + + extraSz = BuildDirectTcpipExtra("127.0.0.1", 8080, "127.0.0.1", 2222, + extra, sizeof(extra)); + inSz = BuildChannelOpenPacket("direct-tcpip", 9, 0x4000, 0x8000, + extra, extraSz, in, sizeof(in)); + + InitChannelOpenHarness(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, RecordingRejectSetupFwdCb, + NULL), WS_SUCCESS); + + ret = DoReceive(harness.ssh); + AssertChannelOpenFailResponse(&harness, ret); + AssertIntEQ(harness.ssh->channelListSz, 0); + AssertIntEQ(fwdActionCount, 1); + AssertIntEQ(fwdActions[0], WOLFSSH_FWD_LOCAL_SETUP); + + FreeChannelOpenHarness(&harness); +} + +/* Opens a direct-tcpip forward through the recording callback and checks + * the setup and channel-id handoff went out, leaving the channel live. */ +static void OpenRecordedDirectTcpip(ChannelOpenHarness* harness, + byte* in, word32 inSz) +{ + byte extra[128]; + word32 extraSz; + + fwdActionCount = 0; + + extraSz = BuildDirectTcpipExtra("127.0.0.1", 8080, "127.0.0.1", 2222, + extra, sizeof(extra)); + inSz = BuildChannelOpenPacket("direct-tcpip", 9, 0x4000, 0x8000, + extra, extraSz, in, inSz); + + InitChannelOpenHarness(harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness->ctx, RecordingFwdCb, NULL), + WS_SUCCESS); + + AssertIntEQ(DoReceive(harness->ssh), WS_SUCCESS); + AssertIntEQ(harness->ssh->channelListSz, 1); + AssertIntEQ(fwdActionCount, 2); + AssertIntEQ(fwdActions[0], WOLFSSH_FWD_LOCAL_SETUP); + AssertIntEQ(fwdActions[1], WOLFSSH_FWD_CHANNEL_ID); +} + +/* A dropped transport never delivers a CHANNEL_CLOSE, so freeing the session + * is the only chance the application gets to release a forward's socket. */ +static void TestSessionFreeSendsLocalCleanup(void) +{ + ChannelOpenHarness harness; + byte in[192]; + + OpenRecordedDirectTcpip(&harness, in, sizeof(in)); + + FreeChannelOpenHarness(&harness); + AssertIntEQ(fwdActionCount, 3); + AssertIntEQ(fwdActions[2], WOLFSSH_FWD_LOCAL_CLEANUP); +} + +/* Same for a channel the application retires itself, and only once: the + * session free that follows must not report it again. */ +static void TestChannelFreeSendsLocalCleanup(void) +{ + ChannelOpenHarness harness; + byte in[192]; + + OpenRecordedDirectTcpip(&harness, in, sizeof(in)); + + AssertIntEQ(wolfSSH_ChannelFree(harness.ssh->channelList), WS_SUCCESS); + AssertIntEQ(harness.ssh->channelListSz, 0); + AssertIntEQ(fwdActionCount, 3); + AssertIntEQ(fwdActions[2], WOLFSSH_FWD_LOCAL_CLEANUP); FreeChannelOpenHarness(&harness); + AssertIntEQ(fwdActionCount, 3); } static void TestForwardedTcpipOnServerSendsOpenFail(void) @@ -13133,6 +13400,13 @@ int main(int argc, char** argv) TestDirectTcpipOpenCbRejectBeatsFwdCb(); TestDirectTcpipFwdCbRejectAfterOpenCbAccept(); TestDirectTcpipFwdCbRejectsChannelId(); + TestDirectTcpipCloseSendsLocalCleanup(); + TestLocalForwardCloseSendsNoCleanup(); + TestSessionCloseSendsNoFwdAction(); + TestFailedOpenAfterSetupSendsCleanup(); + TestRejectedSetupSendsNoCleanup(); + TestSessionFreeSendsLocalCleanup(); + TestChannelFreeSendsLocalCleanup(); TestForwardedTcpipOnServerSendsOpenFail(); TestGlobalRequestFwdNoCbSendsFailure(); #ifndef NO_WOLFSSH_CLIENT diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 588e0c867..a8001c5a6 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1375,6 +1375,7 @@ struct WOLFSSH_CHANNEL { byte eofTxd : 1; byte openConfirmed : 1; byte ptyReq : 1; /* flag for if interactive pty request was received */ + byte fwdSetupTxd : 1; /* a LOCAL_SETUP succeeded, a cleanup is owed */ word32 channel; word32 windowSz; word32 maxPacketSz;