From 8f6a8e7cd7e8c7ec2e50079266d720784f78538d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 23 Jul 2026 14:41:57 -0700 Subject: [PATCH 1/2] scp: fix duplicate file header on send - Gate header on new scpFileHeaderSent flag, not scpFileOffset==0 - A send callback returning 0 bytes first no longer re-sends header - Skip zero-length SCP_SEND_FILE send to avoid empty CHANNEL_DATA - Abort on a second consecutive 0-byte send callback return with file data outstanding; skipping the send would otherwise spin SCP_SEND_FILE -> SCP_TRANSFER with no socket I/O - Reset offset/bufferedSz/flags in ScpSourceInit for connection reuse - Document the WS_CallbackScpSend contract, including when a 0 return is valid, next to the typedef - Add test_wolfSSH_SCP_SendZeroFirst regression (func_args scp_send hook) - Zero func_args in kex.c/testsuite.c so the new scp_send field is not read uninitialized Issue: ZD-22176 --- examples/echoserver/echoserver.c | 5 ++ src/internal.c | 2 + src/wolfscp.c | 107 +++++++++++++++--------- tests/api.c | 135 +++++++++++++++++++++++++++++++ tests/kex.c | 3 + tests/testsuite.c | 2 + wolfssh/internal.h | 3 + wolfssh/test.h | 10 +++ wolfssh/wolfscp.h | 13 +++ 9 files changed, 243 insertions(+), 37 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 92ea43bbf..b4c6db032 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -3200,6 +3200,11 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) wolfSSH_SetUserAuthResult(ctx, wsUserAuthResult); wolfSSH_CTX_SetBanner(ctx, echoserverBanner); +#ifdef WOLFSSH_SCP + /* let a test inject a custom scp send callback in place of the default */ + if (serverArgs->scp_send != NULL) + wolfSSH_SetScpSend(ctx, serverArgs->scp_send); +#endif #ifdef WOLFSSH_AGENT wolfSSH_CTX_set_agent_cb(ctx, wolfSSH_AGENT_DefaultActions, NULL); #endif diff --git a/src/internal.c b/src/internal.c index ce9aa30cf..061e9b30d 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1609,6 +1609,8 @@ WOLFSSH* SshInit(WOLFSSH* ssh, WOLFSSH_CTX* ctx) ssh->scpATime = 0; ssh->scpMTime = 0; ssh->scpRequestType = WOLFSSH_SCP_SINGLE_FILE_REQUEST; + ssh->scpFileHeaderSent = 0; + ssh->scpNoProgress = 0; ssh->scpIsRecursive = 0; ssh->scpDirection = WOLFSSH_SCP_DIR_NONE; ssh->scpDirDepth = 0; diff --git a/src/wolfscp.c b/src/wolfscp.c index 441549673..423411cf5 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -387,6 +387,12 @@ static int ScpSourceInit(WOLFSSH* ssh) ssh->scpFileBufferSz = DEFAULT_SCP_BUFFER_SZ; WMEMSET(ssh->scpFileBuffer, 0, DEFAULT_SCP_BUFFER_SZ); + /* reset per-file state so a reused connection starts a fresh transfer */ + ssh->scpFileOffset = 0; + ssh->scpBufferedSz = 0; + ssh->scpFileHeaderSent = 0; + ssh->scpNoProgress = 0; + return WS_SUCCESS; } @@ -673,8 +679,10 @@ int DoScpSource(WOLFSSH* ssh) ssh->scpBufferedSz += ssh->scpConfirm; ssh->scpConfirm = WS_SCP_CONTINUE; - /* only send timestamp and file header first time */ - if (ssh->scpFileOffset == 0) { + /* send timestamp and file header once per file; keying on + * scpFileOffset would resend them when the callback + * returns 0 bytes on its metadata call */ + if (!ssh->scpFileHeaderSent) { if (ssh->scpTimestamp == 1) { ssh->scpState = SCP_SEND_TIMESTAMP; } else { @@ -747,6 +755,7 @@ int DoScpSource(WOLFSSH* ssh) break; } + ssh->scpFileHeaderSent = 1; ssh->scpState = SCP_RECEIVE_CONFIRMATION; ssh->scpNextState = SCP_SEND_FILE; continue; @@ -754,44 +763,54 @@ int DoScpSource(WOLFSSH* ssh) case SCP_SEND_FILE: WLOG(WS_LOG_DEBUG, scpState, "SCP_SEND_FILE"); - ret = ScpStreamSend(ssh, ssh->scpFileBuffer, - ssh->scpBufferedSz); - if (ret == WS_WANT_READ || ret == WS_WANT_WRITE) { - /* ScpStreamSend already drove the worker through any rekey - * or full window; a non-blocking want means the socket is - * not ready. Surface it for the caller to retry without - * closing the file mid-transfer. scpBufferedSz and - * scpFileOffset are preserved for the next call. */ - break; - } - if (ret == WS_EXTDATA) { - _DumpExtendedData(ssh); - continue; - } - if (ret < 0) { - #if !defined(NO_FILESYSTEM) && \ - !defined(WOLFSSH_SCP_USER_CALLBACKS) - /* if the socket send had a fatal error, try to close any - * open file descriptor before exit */ - ScpSendCtx* sendCtx = NULL; - sendCtx = (ScpSendCtx*)wolfSSH_GetScpSendCtx(ssh); - if (sendCtx != NULL) { - WFCLOSE(ssh->fs, sendCtx->fp); - sendCtx->fp = NULL; + /* nothing buffered (send callback returned 0 bytes): skip the + * send so no zero-length CHANNEL_DATA goes on the wire; routing + * below handles the empty buffer */ + if (ssh->scpBufferedSz > 0) { + ret = ScpStreamSend(ssh, ssh->scpFileBuffer, + ssh->scpBufferedSz); + if (ret == WS_WANT_READ || ret == WS_WANT_WRITE) { + /* ScpStreamSend already drove the worker through any + * rekey or full window; a non-blocking want means the + * socket is not ready. Surface it for the caller to + * retry without closing the file mid-transfer. + * scpBufferedSz and scpFileOffset are preserved for the + * next call. */ + break; + } + if (ret == WS_EXTDATA) { + _DumpExtendedData(ssh); + continue; + } + if (ret < 0) { + #if !defined(NO_FILESYSTEM) && \ + !defined(WOLFSSH_SCP_USER_CALLBACKS) + /* if the socket send had a fatal error, try to close any + * open file descriptor before exit */ + ScpSendCtx* sendCtx = NULL; + sendCtx = (ScpSendCtx*)wolfSSH_GetScpSendCtx(ssh); + if (sendCtx != NULL) { + WFCLOSE(ssh->fs, sendCtx->fp); + sendCtx->fp = NULL; + } + #endif + WLOG(WS_LOG_ERROR, scpError, "failed to send file", ret); + break; } - #endif - WLOG(WS_LOG_ERROR, scpError, "failed to send file", ret); - break; - } - ssh->scpFileOffset += ret; - if (ret != (int)ssh->scpBufferedSz) { - /* case where not all of buffer was sent */ - WMEMMOVE(ssh->scpFileBuffer, ssh->scpFileBuffer + ret, - ssh->scpBufferedSz - ret); + ssh->scpFileOffset += ret; + if (ret > 0) { + /* real forward progress, clear the stall detector */ + ssh->scpNoProgress = 0; + } + if (ret != (int)ssh->scpBufferedSz) { + /* case where not all of buffer was sent */ + WMEMMOVE(ssh->scpFileBuffer, ssh->scpFileBuffer + ret, + ssh->scpBufferedSz - ret); + } + ssh->scpBufferedSz -= ret; + ret = WS_SUCCESS; } - ssh->scpBufferedSz -= ret; - ret = WS_SUCCESS; if (ssh->scpBufferedSz > 0) { /* There is still file data in the buffer to send, @@ -800,6 +819,18 @@ int DoScpSource(WOLFSSH* ssh) continue; } else if (ssh->scpFileOffset < ssh->scpFileSz) { + /* A send callback may hand back 0 bytes once, on the call + * that only fills in metadata. Twice running with file + * data still outstanding means it cannot make progress, + * and looping back to SCP_TRANSFER would spin here with no + * socket I/O at all, so fail instead. */ + if (ssh->scpNoProgress) { + WLOG(WS_LOG_ERROR, scpError, + "send callback made no progress", WS_SCP_ABORT); + ret = WS_SCP_ABORT; + break; + } + ssh->scpNoProgress = 1; ssh->scpState = SCP_TRANSFER; ssh->scpRequestType = WOLFSSH_SCP_CONTINUE_FILE_TRANSFER; @@ -808,6 +839,8 @@ int DoScpSource(WOLFSSH* ssh) if (ssh->scpIsRecursive) { ssh->scpFileOffset = 0; ssh->scpBufferedSz = 0; + ssh->scpFileHeaderSent = 0; + ssh->scpNoProgress = 0; ssh->scpATime = 0; ssh->scpMTime = 0; ssh->scpNextState = SCP_TRANSFER; diff --git a/tests/api.c b/tests/api.c index 6b48184ef..dafe4ae68 100644 --- a/tests/api.c +++ b/tests/api.c @@ -4035,12 +4035,146 @@ static void test_wolfSSH_SCP_ReKey_ToServer_NonBlock(void) scp_rekey_test(1, 1); } +/* A send callback that returns 0 bytes on its first + * WOLFSSH_SCP_SINGLE_FILE_REQUEST (metadata now, data on the following call) + * must not make the server send the file header twice. The offset is static + * because the one-shot echoserver runs a single transfer. */ +static byte scpZeroFirstData[SCP_REKEY_FILE_SZ]; +static word32 scpZeroFirstOffset; + +static int scpSendZeroFirst(WOLFSSH* ssh, int state, const char* peerRequest, + char* fileName, word32 fileNameSz, word64* mTime, word64* aTime, + int* fileMode, word32 fileOffset, word32* totalFileSz, + byte* buf, word32 bufSz, void* ctx) +{ + word32 remain, n; + + (void)ssh; + (void)peerRequest; + (void)fileOffset; + (void)ctx; + + switch (state) { + case WOLFSSH_SCP_NEW_REQUEST: + return WS_SUCCESS; + + case WOLFSSH_SCP_SINGLE_FILE_REQUEST: + /* fill metadata, but hand back zero data bytes on this first call */ + WSTRNCPY(fileName, "scp_hdr_zero.txt", fileNameSz); + if (totalFileSz != NULL) *totalFileSz = SCP_REKEY_FILE_SZ; + if (mTime != NULL) *mTime = 0; + if (aTime != NULL) *aTime = 0; + if (fileMode != NULL) *fileMode = 0644; + scpZeroFirstOffset = 0; + return 0; + + case WOLFSSH_SCP_CONTINUE_FILE_TRANSFER: + remain = SCP_REKEY_FILE_SZ - scpZeroFirstOffset; + if (remain == 0) + return WS_SCP_COMPLETE; + n = (remain < bufSz) ? remain : bufSz; + WMEMCPY(buf, scpZeroFirstData + scpZeroFirstOffset, n); + scpZeroFirstOffset += n; + return (int)n; + + default: + return WS_SCP_ABORT; + } +} + +static void test_wolfSSH_SCP_SendZeroFirst(void) +{ + func_args ser; + tcp_ready ready; + int argsCount; + int ret; + word32 i; + WS_SOCKET_T clientFd; +#ifdef USE_WINDOWS_API + DWORD rcvTimeout = 20000; +#else + struct timeval rcvTimeout; +#endif + const char* args[6]; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + const char* srcName = "./scp_hdr_src.txt"; + const char* fromName = "./scp_hdr_from.txt"; + char srcBuf[32]; + char fromBuf[32]; + char cmd[64]; + THREAD_TYPE serThread; + + WSTRNCPY(srcBuf, srcName, sizeof(srcBuf)); + WSTRNCPY(fromBuf, fromName, sizeof(fromBuf)); + + for (i = 0; i < SCP_REKEY_FILE_SZ; i++) + scpZeroFirstData[i] = (byte)((i * 7 + 1) & 0xff); + /* The on-disk file only satisfies the server's base-path parsing; the + * custom callback supplies the actual bytes, so a duplicated header shows + * up as a content mismatch below rather than a missing file. */ + AssertIntEQ(scpWriteTestFile(srcName, scpZeroFirstData, SCP_REKEY_FILE_SZ), + 0); + + WMEMSET(&ser, 0, sizeof(func_args)); + argsCount = 0; + args[argsCount++] = "."; + args[argsCount++] = "-1"; + args[argsCount++] = "-p"; + args[argsCount++] = "0"; + ser.argv = (char**)args; + ser.argc = argsCount; + ser.signal = &ready; + ser.scp_send = scpSendZeroFirst; + InitTcpReady(ser.signal); + ThreadStart(echoserver_test, (void*)&ser, &serThread); + WaitTcpReady(&ready); + + WSNPRINTF(cmd, sizeof(cmd), "scp -f %s", srcName); + scp_client_connect(&ctx, &ssh, ready.port, cmd); + AssertNotNull(ctx); + AssertNotNull(ssh); + + /* bound the recv so a regression fails the match assert below, not CI */ + clientFd = wolfSSH_get_fd(ssh); +#ifdef USE_WINDOWS_API + (void)setsockopt(clientFd, SOL_SOCKET, SO_RCVTIMEO, + (const char*)&rcvTimeout, sizeof(rcvTimeout)); +#else + rcvTimeout.tv_sec = 20; + rcvTimeout.tv_usec = 0; + (void)setsockopt(clientFd, SOL_SOCKET, SO_RCVTIMEO, + &rcvTimeout, sizeof(rcvTimeout)); +#endif + + ret = wolfSSH_SCP_from(ssh, srcBuf, fromBuf); + AssertIntEQ(ret, WS_SUCCESS); + + ret = wolfSSH_shutdown(ssh); + (void)ret; + + clientFd = wolfSSH_get_fd(ssh); + WCLOSESOCKET(clientFd); + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + ThreadJoin(serThread); + FreeTcpReady(&ready); + + /* a duplicate header would corrupt the stream; an exact match proves the + * header was sent once */ + AssertIntEQ(scpFilesMatch(fromName, scpZeroFirstData, SCP_REKEY_FILE_SZ), 0); + + WREMOVE(NULL, srcName); + WREMOVE(NULL, fromName); +} + #else /* WOLFSSH_SCP && !NO_WOLFSSH_CLIENT && !SINGLE_THREADED && * !NO_FILESYSTEM && !WOLFSSH_SCP_USER_CALLBACKS && !WOLFSSH_ZEPHYR */ static void test_wolfSSH_SCP_ReKey(void) { ; } static void test_wolfSSH_SCP_ReKey_NonBlock(void) { ; } static void test_wolfSSH_SCP_ReKey_ToServer(void) { ; } static void test_wolfSSH_SCP_ReKey_ToServer_NonBlock(void) { ; } +static void test_wolfSSH_SCP_SendZeroFirst(void) { ; } #endif @@ -5242,6 +5376,7 @@ int wolfSSH_ApiTest(int argc, char** argv) test_wolfSSH_SCP_ReKey_NonBlock(); test_wolfSSH_SCP_ReKey_ToServer(); test_wolfSSH_SCP_ReKey_ToServer_NonBlock(); + test_wolfSSH_SCP_SendZeroFirst(); /* SFTP tests */ test_wolfSSH_SFTP_SendReadPacket(); diff --git a/tests/kex.c b/tests/kex.c index b08560cf7..c3cd3a9b8 100644 --- a/tests/kex.c +++ b/tests/kex.c @@ -236,6 +236,7 @@ static int wolfSSH_KexTest_Connect(const char* kex) InitTcpReady(&ready); + WMEMSET(&serverArgs, 0, sizeof(serverArgs)); ADD_ARG(serverArgv, serverArgc, "echoserver"); ADD_ARG(serverArgv, serverArgc, "-1"); ADD_ARG(serverArgv, serverArgc, "-f"); @@ -326,6 +327,7 @@ static int wolfSSH_KexTest_Ed25519HostKey(void) InitTcpReady(&ready); + WMEMSET(&serverArgs, 0, sizeof(serverArgs)); ADD_ARG(serverArgv, serverArgc, "echoserver"); ADD_ARG(serverArgv, serverArgc, "-1"); ADD_ARG(serverArgv, serverArgc, "-f"); @@ -401,6 +403,7 @@ static int wolfSSH_KexTest_MlDsaHostKey(const char* keyName) InitTcpReady(&ready); + WMEMSET(&serverArgs, 0, sizeof(serverArgs)); ADD_ARG(serverArgv, serverArgc, "echoserver"); ADD_ARG(serverArgv, serverArgc, "-1"); ADD_ARG(serverArgv, serverArgc, "-f"); diff --git a/tests/testsuite.c b/tests/testsuite.c index 68fd73ecb..77764bdba 100644 --- a/tests/testsuite.c +++ b/tests/testsuite.c @@ -110,6 +110,7 @@ static void wolfSSH_EchoTest(void) InitTcpReady(&ready); + WMEMSET(&serverArgs, 0, sizeof(serverArgs)); WSTRNCPY(serverArgv[serverArgc++], "echoserver", ARGLEN); WSTRNCPY(serverArgv[serverArgc++], "-1", ARGLEN); WSTRNCPY(serverArgv[serverArgc++], "-f", ARGLEN); @@ -168,6 +169,7 @@ static void wolfSSH_EchoTest_MultiPubKey(void) InitTcpReady(&ready); + WMEMSET(&serverArgs, 0, sizeof(serverArgs)); WSTRNCPY(serverArgv[serverArgc++], "echoserver", ARGLEN); WSTRNCPY(serverArgv[serverArgc++], "-1", ARGLEN); WSTRNCPY(serverArgv[serverArgc++], "-f", ARGLEN); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 84a2ea3c5..1755c767e 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1003,6 +1003,9 @@ struct WOLFSSH { word32 scpFileBufferSz; /* size of transfer buffer, octets */ word32 scpFileOffset; /* current offset into file transfer */ word32 scpBufferedSz; /* bytes buffered to send to peer */ + byte scpFileHeaderSent; /* file header already sent for current file */ + byte scpNoProgress; /* send callback already handed back 0 bytes + * with file data still outstanding */ word32 scpDirDepth; /* SCP nested NEW_DIR depth below base path */ #ifdef WOLFSSL_NUCLEUS int scpFd; /* SCP receive callback context handle */ diff --git a/wolfssh/test.h b/wolfssh/test.h index bfadb703d..f1bc19159 100644 --- a/wolfssh/test.h +++ b/wolfssh/test.h @@ -41,6 +41,12 @@ /*#include */ #endif +#ifdef WOLFSSH_SCP + /* for WS_CallbackScpSend used in func_args; test.h is included by sources + * (e.g. testsuite.c) that do not otherwise pull in wolfscp.h */ + #include +#endif + #ifdef USE_WINDOWS_API #ifndef _WIN32_WCE #include @@ -857,6 +863,10 @@ typedef struct func_args { /* callback for example sftp client commands instead of WFGETS */ WS_CallbackSftpCommand sftp_cb; #endif +#ifdef WOLFSSH_SCP + /* override the server's default scp send callback (test injection) */ + WS_CallbackScpSend scp_send; +#endif } func_args; diff --git a/wolfssh/wolfscp.h b/wolfssh/wolfscp.h index e26942fda..afa67e599 100644 --- a/wolfssh/wolfscp.h +++ b/wolfssh/wolfscp.h @@ -121,6 +121,19 @@ typedef int (*WS_CallbackScpRecv)(WOLFSSH* ssh, int state, int fileMode, word64 mTime, word64 aTime, word32 totalFileSz, byte* buf, word32 bufSz, word32 fileOffset, void* ctx); +/* Send callback. Returns the number of octets placed in "buf", or one of the + * WS_SCP_* status codes, or a negative error to abort the transfer. + * + * "fileNameSz" is the capacity of the "fileName" buffer, not the length of any + * name already in it. + * + * Returning 0 is only valid on the call that fills in metadata (file name, + * mode, times, totalFileSz) and has no data ready yet; the library then sends + * the file header and calls back with WOLFSSH_SCP_CONTINUE_FILE_TRANSFER. A + * second 0 in a row while "fileOffset" is still short of "totalFileSz" is + * treated as a stalled callback and aborts the transfer, since there is no + * "no data right now" status in this API. Callbacks that can block should do + * so rather than returning 0. */ typedef int (*WS_CallbackScpSend)(WOLFSSH* ssh, int state, const char* peerRequest, char* fileName, word32 fileNameSz, word64* mTime, From c442e4b170d846e14896856b4c598ceb0a135306 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 30 Jul 2026 11:43:27 -0700 Subject: [PATCH 2/2] scp: reuse GetScpFileName buffer instead of realloc - Track the scpFileName allocation size in a new scpFileNameCap field, so GetScpFileName() and ScpCheckForRename() reuse the buffer whenever the name plus its terminator fits, instead of testing against the previous name length - Pass scpFileNameCap to the send callback, which writes into scpFileName and needs the capacity; scpFileNameSz is now the name length on every path, including the source path - Free and reallocate the transfer buffer in ScpSourceInit(), clearing the size fields alongside the pointers they describe - Wrap a long line in ScpProcessEntry() - Add test_ScpGetFileName covering the reuse-vs-realloc branch, the exact-fit boundary, a grow-by-one that catches an off-by-one in the reuse condition, and a source-path buffer holding no name yet - Add test_wolfSSH_SCP_RecursiveTwoFiles, a real "scp -r" transfer of two files, covering the scpFileHeaderSent reset on the recursive path added in the duplicate-header fix; it clears leftovers from an aborted run up front and bounds its recv so a regression fails instead of hanging. Not built on Windows, where a recursive transfer through the default callbacks does not reproduce the sent file --- src/internal.c | 2 + src/wolfscp.c | 61 +++++++++++--- tests/api.c | 137 +++++++++++++++++++++++++++++++ tests/unit.c | 196 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 4 + 5 files changed, 387 insertions(+), 13 deletions(-) diff --git a/src/internal.c b/src/internal.c index 061e9b30d..8749a2bf8 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1605,6 +1605,7 @@ WOLFSSH* SshInit(WOLFSSH* ssh, WOLFSSH_CTX* ctx) ssh->scpFileBufferSz = 0; ssh->scpFileName = NULL; ssh->scpFileNameSz = 0; + ssh->scpFileNameCap = 0; ssh->scpTimestamp = 0; ssh->scpATime = 0; ssh->scpMTime = 0; @@ -1706,6 +1707,7 @@ void SshResourceFree(WOLFSSH* ssh, void* heap) WFREE(ssh->scpFileName, heap, DYNTYPE_STRING); ssh->scpFileName = NULL; ssh->scpFileNameSz = 0; + ssh->scpFileNameCap = 0; } if (ssh->scpRecvMsg) { WFREE(ssh->scpRecvMsg, heap, DYNTYPE_STRING); diff --git a/src/wolfscp.c b/src/wolfscp.c index 423411cf5..d2f2b53f4 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -366,6 +366,7 @@ static int ScpSourceInit(WOLFSSH* ssh) WFREE(ssh->scpFileName, ssh->ctx->heap, DYNTYPE_STRING); ssh->scpFileName = NULL; ssh->scpFileNameSz = 0; + ssh->scpFileNameCap = 0; } ssh->scpFileName = (char*)WMALLOC(DEFAULT_SCP_FILE_NAME_SZ, ssh->ctx->heap, @@ -373,17 +374,28 @@ static int ScpSourceInit(WOLFSSH* ssh) if (ssh->scpFileName == NULL) return WS_MEMORY_E; - ssh->scpFileNameSz = DEFAULT_SCP_FILE_NAME_SZ; + /* The source path uses scpFileName as a fixed-size scratch buffer that the + * send callback fills in, so there is no name in it yet. */ + ssh->scpFileNameCap = DEFAULT_SCP_FILE_NAME_SZ; + ssh->scpFileNameSz = 0; WMEMSET(ssh->scpFileName, 0, DEFAULT_SCP_FILE_NAME_SZ); /* file buffer */ + if (ssh->scpFileBuffer != NULL) { + WFREE(ssh->scpFileBuffer, ssh->ctx->heap, DYNTYPE_BUFFER); + ssh->scpFileBuffer = NULL; + ssh->scpFileBufferSz = 0; + } + ssh->scpFileBuffer = (byte*)WMALLOC(DEFAULT_SCP_BUFFER_SZ, ssh->ctx->heap, DYNTYPE_BUFFER); if (ssh->scpFileBuffer == NULL) { WFREE(ssh->scpFileName, ssh->ctx->heap, DYNTYPE_STRING); ssh->scpFileName = NULL; + ssh->scpFileNameCap = 0; return WS_MEMORY_E; } + ssh->scpFileBufferSz = DEFAULT_SCP_BUFFER_SZ; WMEMSET(ssh->scpFileBuffer, 0, DEFAULT_SCP_BUFFER_SZ); @@ -633,9 +645,12 @@ int DoScpSource(WOLFSSH* ssh) case SCP_TRANSFER: WLOG(WS_LOG_DEBUG, scpState, "SCP_TRANSFER"); + /* the callback writes the name into scpFileName, so it needs + * the buffer capacity, not the current name length */ ssh->scpConfirm = ssh->ctx->scpSendCb(ssh, ssh->scpRequestType, ssh->scpBasePath, - ssh->scpFileName, ssh->scpFileNameSz, &(ssh->scpMTime), + ssh->scpFileName, ssh->scpFileNameCap, + &(ssh->scpMTime), &(ssh->scpATime), &(ssh->scpFileMode), ssh->scpFileOffset, &(ssh->scpFileSz), ssh->scpFileBuffer + ssh->scpBufferedSz, @@ -1258,16 +1273,24 @@ static int GetScpFileName(WOLFSSH* ssh, byte* buf, word32 bufSz, } } - if (ssh->scpFileName != NULL) { - WFREE(ssh->scpFileName, ssh->ctx->heap, DYNTYPE_STRING); - ssh->scpFileName = NULL; - ssh->scpFileNameSz = 0; - } + /* reuse the existing allocation when the name plus its terminator + * fits; scpFileNameCap is the allocation size, so this is correct no + * matter what the last name length was */ + if (ssh->scpFileName == NULL || ssh->scpFileNameCap <= len) { + if (ssh->scpFileName != NULL) { + WFREE(ssh->scpFileName, ssh->ctx->heap, DYNTYPE_STRING); + ssh->scpFileName = NULL; + ssh->scpFileNameSz = 0; + ssh->scpFileNameCap = 0; + } - ssh->scpFileName = (char*)WMALLOC(len + 1, ssh->ctx->heap, - DYNTYPE_STRING); - if (ssh->scpFileName == NULL) - ret = WS_MEMORY_E; + ssh->scpFileName = (char*)WMALLOC(len + 1, ssh->ctx->heap, + DYNTYPE_STRING); + if (ssh->scpFileName == NULL) + ret = WS_MEMORY_E; + else + ssh->scpFileNameCap = len + 1; + } if (ret == WS_SUCCESS) { WMEMCPY(ssh->scpFileName, buf + idx, len); @@ -1281,6 +1304,14 @@ static int GetScpFileName(WOLFSSH* ssh, byte* buf, word32 bufSz, return ret; } +#ifdef WOLFSSH_TEST_INTERNAL +int wolfSSH_TestScpGetFileName(WOLFSSH* ssh, byte* buf, word32 bufSz, + word32* inOutIdx) +{ + return GetScpFileName(ssh, buf, bufSz, inOutIdx); +} +#endif /* WOLFSSH_TEST_INTERNAL */ + /* Reads timestamp information (access, modification) from beginning * of string, expects space to be after each time value: * @@ -1443,10 +1474,12 @@ static int ScpCheckForRename(WOLFSSH* ssh) } sz = sz - idx; /* size of file name */ - if (ssh->scpFileNameSz < (word32)sz || ssh->scpFileName == NULL) { + if (ssh->scpFileName == NULL || ssh->scpFileNameCap <= (word32)sz) { if (ssh->scpFileName != NULL) { WFREE(ssh->scpFileName, ssh->ctx->heap, DYNTYPE_STRING); + ssh->scpFileName = NULL; ssh->scpFileNameSz = 0; + ssh->scpFileNameCap = 0; } ssh->scpFileName = (char*)WMALLOC(sz + 1, ssh->ctx->heap, DYNTYPE_STRING); @@ -1456,6 +1489,7 @@ static int ScpCheckForRename(WOLFSSH* ssh) ssh->scpBasePath = NULL; return WS_MEMORY_E; } + ssh->scpFileNameCap = sz + 1; ssh->scpFileName[0] = '\0'; /* make sure null terminated for check */ } @@ -3057,7 +3091,8 @@ static int ScpProcessEntry(WOLFSSH* ssh, char* fileName, word64* mTime, DEFAULT_SCP_FILE_NAME_SZ); WSTRNCPY(fileName, sendCtx->currentDir->dir.lfname, DEFAULT_SCP_FILE_NAME_SZ); - if (wolfSSH_CleanPath(ssh, filePath, DEFAULT_SCP_FILE_NAME_SZ) < 0) { + if (wolfSSH_CleanPath(ssh, filePath, + DEFAULT_SCP_FILE_NAME_SZ) < 0) { ret = WS_SCP_ABORT; } #elif defined(USE_WINDOWS_API) diff --git a/tests/api.c b/tests/api.c index dafe4ae68..c4a64d87a 100644 --- a/tests/api.c +++ b/tests/api.c @@ -4082,6 +4082,141 @@ static int scpSendZeroFirst(WOLFSSH* ssh, int state, const char* peerRequest, } } +/* Drives a real "scp -r" of a directory holding two files through the + * default filesystem send/recv callbacks. The header-dedup fix gates + * sending a file's header on a scpFileHeaderSent flag that gets reset when + * SCP_SEND_FILE loops back to SCP_TRANSFER for the next file in a recursive + * copy; this confirms that reset lets the second file get its own header + * instead of it being duplicated or skipped. + * + * Not run on Windows: this is the only end-to-end exercise of a recursive + * transfer anywhere in the suite (the example client cannot issue "scp -r -f", + * so scripts/scp.test never reaches it), and the received file does not match + * there. That is a Windows-side recursive SCP problem of its own, unrelated to + * the header fix, which reproduces on the default callbacks and needs its own + * investigation. */ +#ifndef USE_WINDOWS_API +static void test_wolfSSH_SCP_RecursiveTwoFiles(void) +{ + func_args ser; + tcp_ready ready; + int argsCount; + int ret; + word32 i; + WS_SOCKET_T clientFd; +#ifdef USE_WINDOWS_API + DWORD rcvTimeout = 20000; +#else + struct timeval rcvTimeout; +#endif + const char* args[6]; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + const char* dstDir = "./scp_recur_dst"; + const char* out1 = "./scp_recur_dst/a_short.txt"; + const char* out2 = "./scp_recur_dst/b_longer_name.txt"; + byte data1[300]; + byte data2[700]; + char cmd[300]; + char cwdBuf[200]; + /* must hold all of srcBuf plus the longest name below, or gcc rejects the + * WSNPRINTF calls under -Werror=format-truncation */ + char file1[300]; + char file2[300]; + /* wolfSSH_SCP_from() mutates its src/dst buffers in place (e.g. + * ScpCheckForRename() writes a NUL into the path), so these cannot be + * string literals. The client thread chdir()s into dstDir while + * receiving a directory; since the client and server here share one + * process (and thus one cwd), srcBuf must be absolute so the server + * thread's concurrent directory walk does not resolve relative to + * whatever directory the client just chdir()ed into. */ + char srcBuf[256]; + char dstBuf[32]; + THREAD_TYPE serThread; + + for (i = 0; i < sizeof(data1); i++) + data1[i] = (byte)((i * 3 + 1) & 0xff); + for (i = 0; i < sizeof(data2); i++) + data2[i] = (byte)((i * 5 + 2) & 0xff); + + AssertNotNull(WGETCWD(NULL, cwdBuf, sizeof(cwdBuf))); + WSNPRINTF(srcBuf, sizeof(srcBuf), "%s/scp_recur_src", cwdBuf); + WSTRNCPY(dstBuf, dstDir, sizeof(dstBuf)); + WSNPRINTF(file1, sizeof(file1), "%s/a_short.txt", srcBuf); + WSNPRINTF(file2, sizeof(file2), "%s/b_longer_name.txt", srcBuf); + + /* full teardown first: a run that aborted mid-transfer leaves these + * behind, and then WMKDIR below fails with EEXIST, masking the real + * failure with a setup error */ + WREMOVE(NULL, file1); + WREMOVE(NULL, file2); + WRMDIR(NULL, srcBuf); + WREMOVE(NULL, out1); + WREMOVE(NULL, out2); + WRMDIR(NULL, dstDir); + + AssertIntEQ(WMKDIR(NULL, srcBuf, 0700), 0); + AssertIntEQ(scpWriteTestFile(file1, data1, sizeof(data1)), 0); + AssertIntEQ(scpWriteTestFile(file2, data2, sizeof(data2)), 0); + + WMEMSET(&ser, 0, sizeof(func_args)); + argsCount = 0; + args[argsCount++] = "."; + args[argsCount++] = "-1"; + args[argsCount++] = "-p"; + args[argsCount++] = "0"; + ser.argv = (char**)args; + ser.argc = argsCount; + ser.signal = &ready; + InitTcpReady(ser.signal); + ThreadStart(echoserver_test, (void*)&ser, &serThread); + WaitTcpReady(&ready); + + WSNPRINTF(cmd, sizeof(cmd), "scp -r -f %s", srcBuf); + scp_client_connect(&ctx, &ssh, ready.port, cmd); + AssertNotNull(ctx); + AssertNotNull(ssh); + + /* bound the recv so a regression fails the match assert below, not CI */ + clientFd = wolfSSH_get_fd(ssh); +#ifdef USE_WINDOWS_API + (void)setsockopt(clientFd, SOL_SOCKET, SO_RCVTIMEO, + (const char*)&rcvTimeout, sizeof(rcvTimeout)); +#else + rcvTimeout.tv_sec = 20; + rcvTimeout.tv_usec = 0; + (void)setsockopt(clientFd, SOL_SOCKET, SO_RCVTIMEO, + &rcvTimeout, sizeof(rcvTimeout)); +#endif + + ret = wolfSSH_SCP_from(ssh, srcBuf, dstBuf); + AssertIntEQ(ret, WS_SUCCESS); + + ret = wolfSSH_shutdown(ssh); + (void)ret; + + WCLOSESOCKET(wolfSSH_get_fd(ssh)); + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + ThreadJoin(serThread); + FreeTcpReady(&ready); + + /* a duplicated or skipped header on the second file corrupts the byte + * stream; an exact match on both files proves each got its own header */ + AssertIntEQ(scpFilesMatch(out1, data1, sizeof(data1)), 0); + AssertIntEQ(scpFilesMatch(out2, data2, sizeof(data2)), 0); + + WREMOVE(NULL, file1); + WREMOVE(NULL, file2); + WRMDIR(NULL, srcBuf); + WREMOVE(NULL, out1); + WREMOVE(NULL, out2); + WRMDIR(NULL, dstDir); +} +#else +static void test_wolfSSH_SCP_RecursiveTwoFiles(void) { ; } +#endif /* USE_WINDOWS_API */ + static void test_wolfSSH_SCP_SendZeroFirst(void) { func_args ser; @@ -4175,6 +4310,7 @@ static void test_wolfSSH_SCP_ReKey_NonBlock(void) { ; } static void test_wolfSSH_SCP_ReKey_ToServer(void) { ; } static void test_wolfSSH_SCP_ReKey_ToServer_NonBlock(void) { ; } static void test_wolfSSH_SCP_SendZeroFirst(void) { ; } +static void test_wolfSSH_SCP_RecursiveTwoFiles(void) { ; } #endif @@ -5377,6 +5513,7 @@ int wolfSSH_ApiTest(int argc, char** argv) test_wolfSSH_SCP_ReKey_ToServer(); test_wolfSSH_SCP_ReKey_ToServer_NonBlock(); test_wolfSSH_SCP_SendZeroFirst(); + test_wolfSSH_SCP_RecursiveTwoFiles(); /* SFTP tests */ test_wolfSSH_SFTP_SendReadPacket(); diff --git a/tests/unit.c b/tests/unit.c index 441054fd0..11b5bd0a0 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -2940,6 +2940,198 @@ static int test_ScpGetTimestamp(void) wolfSSH_CTX_free(ctx); return result; } + +/* Drive GetScpFileName directly, covering the reuse-vs-realloc branch added + * to avoid a free/WMALLOC pair on every call: a name that fits within the + * previous allocation reuses ssh->scpFileName (verified by pointer identity), + * while a name too long for it forces a fresh allocation. Each case also + * checks scpFileNameCap, which is the allocation size the reuse condition + * tests against, so an off-by-one there fails deterministically rather than + * only under a heap checker. */ +static int test_ScpGetFileName(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + char* firstAlloc = NULL; + char* afterGrow = NULL; + char tmp[64]; + int result = 0; + int ret; + word32 idx; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -450; + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { + wolfSSH_CTX_free(ctx); + return -451; + } + + /* first call: nothing allocated yet, must allocate */ + WSTRNCPY(tmp, "short.txt", sizeof(tmp)); + idx = 0; + ret = wolfSSH_TestScpGetFileName(ssh, (byte*)tmp, (word32)WSTRLEN(tmp), + &idx); + if (ret != WS_SUCCESS) + result = -452; + else if (ssh->scpFileName == NULL) + result = -453; + else if (WSTRNCMP(ssh->scpFileName, "short.txt", 10) != 0) + result = -454; + else if (ssh->scpFileNameSz != 9) + result = -455; + else if (ssh->scpFileNameCap != 10) + result = -476; + firstAlloc = ssh->scpFileName; + + /* second call: shorter name fits in the existing allocation, reuse it */ + if (result == 0) { + WSTRNCPY(tmp, "ab", sizeof(tmp)); + idx = 0; + ret = wolfSSH_TestScpGetFileName(ssh, (byte*)tmp, + (word32)WSTRLEN(tmp), &idx); + if (ret != WS_SUCCESS) + result = -456; + else if (ssh->scpFileName != firstAlloc) + result = -457; + else if (WSTRNCMP(ssh->scpFileName, "ab", 3) != 0) + result = -458; + else if (ssh->scpFileNameSz != 2) + result = -459; + } + + /* third call: a name as long as the last one is still reused, because the + * check is against the allocation size and not the previous name length */ + if (result == 0) { + WSTRNCPY(tmp, "xy", sizeof(tmp)); + idx = 0; + ret = wolfSSH_TestScpGetFileName(ssh, (byte*)tmp, + (word32)WSTRLEN(tmp), &idx); + if (ret != WS_SUCCESS) + result = -460; + else if (ssh->scpFileName != firstAlloc) + result = -461; + else if (WSTRNCMP(ssh->scpFileName, "xy", 3) != 0) + result = -462; + else if (ssh->scpFileNameCap != 10) + result = -477; + } + + /* fourth call: a name longer than the current allocation forces a fresh + * WMALLOC */ + if (result == 0) { + WSTRNCPY(tmp, "a-name-considerably-longer-than-before.bin", + sizeof(tmp)); + idx = 0; + ret = wolfSSH_TestScpGetFileName(ssh, (byte*)tmp, + (word32)WSTRLEN(tmp), &idx); + if (ret != WS_SUCCESS) + result = -463; + else if (WSTRNCMP(ssh->scpFileName, + "a-name-considerably-longer-than-before.bin", 43) != 0) + result = -464; + else if (ssh->scpFileNameSz != 42) + result = -465; + else if (ssh->scpFileNameCap != 43) + result = -478; + afterGrow = ssh->scpFileName; + } + + /* fifth call: tight-fit boundary. The allocation is exactly 43 bytes + * here, so reusing it for a 42-character name puts the NUL on the + * buffer's final byte -- catches an off-by-one in the WMALLOC size. */ + if (result == 0) { + WSTRNCPY(tmp, "a-different-name-of-the-very-same-length.b", + sizeof(tmp)); + idx = 0; + ret = wolfSSH_TestScpGetFileName(ssh, (byte*)tmp, + (word32)WSTRLEN(tmp), &idx); + if (ret != WS_SUCCESS) + result = -469; + else if (ssh->scpFileName != afterGrow) + result = -470; + else if (WSTRNCMP(ssh->scpFileName, + "a-different-name-of-the-very-same-length.b", 43) != 0) + result = -471; + else if (ssh->scpFileNameSz != 42) + result = -472; + else if (ssh->scpFileNameCap != 43) + result = -479; + } + + /* sixth call: grow by exactly one byte. A 43-character name needs 44 + * bytes, one more than the current allocation, so it must not be reused. + * The scpFileNameCap check below is what catches an off-by-one in the + * reuse condition itself: a loosened condition would reuse the 43-byte + * block and write the NUL one past its end, which the content and + * scpFileNameSz checks cannot see. */ + if (result == 0) { + WSTRNCPY(tmp, "another-42-char-name-for-tight-fit-case.bin", + sizeof(tmp)); + idx = 0; + ret = wolfSSH_TestScpGetFileName(ssh, (byte*)tmp, + (word32)WSTRLEN(tmp), &idx); + if (ret != WS_SUCCESS) + result = -473; + else if (WSTRNCMP(ssh->scpFileName, + "another-42-char-name-for-tight-fit-case.bin", 44) != 0) + result = -474; + else if (ssh->scpFileNameSz != 43) + result = -475; + else if (ssh->scpFileNameCap != 44) + result = -480; + afterGrow = ssh->scpFileName; + } + + /* seventh call: back to a short name, reuses the grown allocation */ + if (result == 0) { + WSTRNCPY(tmp, "z", sizeof(tmp)); + idx = 0; + ret = wolfSSH_TestScpGetFileName(ssh, (byte*)tmp, + (word32)WSTRLEN(tmp), &idx); + if (ret != WS_SUCCESS) + result = -466; + else if (ssh->scpFileName != afterGrow) + result = -467; + else if (WSTRNCMP(ssh->scpFileName, "z", 2) != 0) + result = -468; + else if (ssh->scpFileNameCap != 44) + result = -481; + } + + /* a source-path session sets scpFileNameCap without ever putting a name in + * the buffer; a name of exactly that capacity must still reallocate rather + * than write its NUL one byte past the end */ + if (result == 0) { + WFREE(ssh->scpFileName, ssh->ctx->heap, DYNTYPE_STRING); + ssh->scpFileName = (char*)WMALLOC(8, ssh->ctx->heap, DYNTYPE_STRING); + if (ssh->scpFileName == NULL) { + result = -482; + } + else { + ssh->scpFileNameCap = 8; + ssh->scpFileNameSz = 0; + + WSTRNCPY(tmp, "12345678", sizeof(tmp)); + idx = 0; + ret = wolfSSH_TestScpGetFileName(ssh, (byte*)tmp, + (word32)WSTRLEN(tmp), &idx); + if (ret != WS_SUCCESS) + result = -483; + else if (ssh->scpFileNameCap != 9) + result = -484; + else if (WSTRNCMP(ssh->scpFileName, "12345678", 9) != 0) + result = -485; + else if (ssh->scpFileNameSz != 8) + result = -486; + } + } + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} #endif /* WOLFSSH_TEST_INTERNAL && WOLFSSH_SCP */ static int test_ChannelPutData(void) @@ -12101,6 +12293,10 @@ int wolfSSH_UnitTest(int argc, char** argv) unitResult = test_ScpGetTimestamp(); printf("ScpGetTimestamp: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + + unitResult = test_ScpGetFileName(); + printf("ScpGetFileName: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; #endif unitResult = test_MsgHighwater(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 1755c767e..917670e99 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -996,6 +996,8 @@ struct WOLFSSH { char* scpFileName; /* file name, dynamic */ char* scpFileReName; /* file rename case, points to scpFileName */ word32 scpFileNameSz; /* length of fileName, not including \0 */ + word32 scpFileNameCap; /* bytes allocated for scpFileName, including + * room for the terminating \0 */ byte scpTimestamp; /* did peer request timestamp? {0:1} */ word64 scpATime; /* scp file access time, secs since epoch */ word64 scpMTime; /* scp file modification time, secs epoch */ @@ -1656,6 +1658,8 @@ enum WS_MessageIdLimits { word32 bufSz, word32* inOutIdx); WOLFSSH_API int wolfSSH_TestScpGetTimestamp(WOLFSSH* ssh, byte* buf, word32 bufSz, word32* inOutIdx); + WOLFSSH_API int wolfSSH_TestScpGetFileName(WOLFSSH* ssh, byte* buf, + word32 bufSz, word32* inOutIdx); #if !defined(WOLFSSH_SCP_USER_CALLBACKS) && !defined(NO_FILESYSTEM) WOLFSSH_API int wolfSSH_TestScpPushDir(const char* path); #endif