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

Author: ejohnstown

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

src/wolfscp.c

@@ -366,24 +366,36 @@ 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,
                                       DYNTYPE_STRING);
     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)

tests/api.c

@@ -4082,6 +4082,128 @@ 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. */
+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];
+    char file1[256];
+    char file2[256];
+    /* 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);
+}
+
 static void test_wolfSSH_SCP_SendZeroFirst(void)
 {
     func_args ser;
@@ -4175,6 +4297,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 +5500,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();