Address items in the review

Author: LinuxJedi

.github/workflows/paramiko-sftp-test.yml

@@ -188,6 +188,8 @@ jobs:
               num_iterations = 10
               timeout_seconds = 30  # Per-operation timeout to detect hangs
 
+              orig_hash = get_file_hash('/tmp/sftp_upload/test_upload.dat')
+
               for i in range(num_iterations):
                   signal.signal(signal.SIGALRM, timeout_handler)
                   signal.alarm(timeout_seconds)
@@ -197,10 +199,8 @@ jobs:
                       # Paramiko uses prefetch by default for get()
                       sftp.get('/tmp/test_upload.dat', download_path)
                       elapsed = time.time() - start_time
-                      signal.alarm(0)  # Cancel alarm
 
                       # Verify integrity
-                      orig_hash = get_file_hash('/tmp/sftp_upload/test_upload.dat')
                       down_hash = get_file_hash(download_path)
                       if orig_hash != down_hash:
                           print(f"  Iteration {i+1}: FAILED - hash mismatch!")
@@ -213,6 +213,8 @@ jobs:
                       print(f"  Iteration {i+1}: FAILED - {e}")
                       print("  This may indicate the WS_WANT_WRITE hang bug!")
                       return False
+                  finally:
+                      signal.alarm(0)  # Always cancel alarm
 
               print(f"=== Stress test completed: {num_iterations} iterations OK ===\n")
 

src/internal.c

@@ -3462,6 +3462,12 @@ int wolfSSH_SendPacket(WOLFSSH* ssh)
 }
 
 
+int wolfSSH_OutputPending(WOLFSSH* ssh)
+{
+    return (ssh != NULL && ssh->outputBuffer.length > ssh->outputBuffer.idx);
+}
+
+
 static int GetInputData(WOLFSSH* ssh, word32 size)
 {
     int in;

src/wolfsftp.c

@@ -145,8 +145,13 @@ enum WS_SFTP_LSTAT_STATE_ID {
     STATE_LSTAT_CLEANUP
 };
 
-/* WS_SFTP_BUFFER is defined in wolfsftp.h for use with nonblocking state.
+/* SFTP buffer for nonblocking state tracking.
  * If adding any read/writes use the wolfSSH_SFTP_buffer_read/send functions */
+typedef struct WS_SFTP_BUFFER {
+    byte*  data;
+    word32 sz;
+    word32 idx;
+} WS_SFTP_BUFFER;
 
 typedef struct WS_SFTP_CHMOD_STATE {
     enum WS_SFTP_CHMOD_STATE_ID state;
@@ -508,7 +513,7 @@ static void wolfSSH_SFTP_buffer_rewind(WS_SFTP_BUFFER* buffer)
 
 /* try to send the rest of the buffer (buffer.sz - buffer.idx)
  * increments idx with amount sent */
-WOLFSSH_LOCAL int wolfSSH_SFTP_buffer_send(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer)
+static int wolfSSH_SFTP_buffer_send(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer)
 {
     int ret = WS_SUCCESS;
     int err;
@@ -524,7 +529,7 @@ WOLFSSH_LOCAL int wolfSSH_SFTP_buffer_send(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer)
     /* Flush any pending data in SSH output buffer first.
      * Handles case where previous send returned WS_WANT_WRITE
      * and data is still buffered at the SSH layer. */
-    if (ssh->outputBuffer.length > ssh->outputBuffer.idx) {
+    if (wolfSSH_OutputPending(ssh)) {
         ret = wolfSSH_SendPacket(ssh);
         if (ret == WS_WANT_WRITE) {
             return ret;
@@ -554,6 +559,20 @@ WOLFSSH_LOCAL int wolfSSH_SFTP_buffer_send(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer)
 }
 
 
+#ifdef WOLFSSH_TEST_INTERNAL
+int wolfSSH_TestSftpBufferSend(WOLFSSH* ssh,
+        byte* data, word32 sz, word32 idx)
+{
+    WS_SFTP_BUFFER buffer;
+
+    buffer.data = data;
+    buffer.sz = sz;
+    buffer.idx = idx;
+    return wolfSSH_SFTP_buffer_send(ssh, &buffer);
+}
+#endif
+
+
 /* returns the amount read on success */
 static int wolfSSH_SFTP_buffer_read(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer,
         int readSz)
@@ -1614,8 +1633,7 @@ int wolfSSH_SFTP_read(WOLFSSH* ssh)
                 /* Check if SSH layer still has pending data from WS_WANT_WRITE.
                  * Even if SFTP buffer is fully consumed, the data may still be
                  * sitting in the SSH output buffer waiting to be sent. */
-                if (ssh->error == WS_WANT_WRITE ||
-                        ssh->outputBuffer.length > ssh->outputBuffer.idx) {
+                if (wolfSSH_OutputPending(ssh)) {
                     ssh->error = WS_WANT_WRITE;
                     return WS_FATAL_ERROR;
                 }

tests/regress.c

@@ -515,7 +515,6 @@ static void TestSftpBufferSendPendingOutput(void)
 {
     WOLFSSH_CTX* ctx;
     WOLFSSH* ssh;
-    WS_SFTP_BUFFER sftpBuf;
     byte testData[16];
     int ret;
 
@@ -528,12 +527,7 @@ static void TestSftpBufferSendPendingOutput(void)
     ssh = wolfSSH_new(ctx);
     AssertNotNull(ssh);
 
-    /* Set up SFTP buffer with some data to send */
     WMEMSET(testData, 0x42, sizeof(testData));
-    WMEMSET(&sftpBuf, 0, sizeof(sftpBuf));
-    sftpBuf.data = testData;
-    sftpBuf.sz = sizeof(testData);
-    sftpBuf.idx = 0;
 
     /* Simulate pending data in SSH output buffer (as if previous send
      * returned WS_WANT_WRITE and data was buffered).
@@ -544,21 +538,16 @@ static void TestSftpBufferSendPendingOutput(void)
 
     sftpWantWriteCallCount = 0;
 
-    /* Call wolfSSH_SFTP_buffer_send - should return WS_WANT_WRITE because
+    /* Call wolfSSH_TestSftpBufferSend - should return WS_WANT_WRITE because
      * the fix detects pending data in outputBuffer and tries to flush it,
      * which fails with WS_WANT_WRITE from our callback.
      *
      * Before the fix, the function would ignore the pending SSH output buffer
      * data and proceed to send new SFTP data, leading to a hang because the
      * pending data was never flushed. */
-    ret = wolfSSH_SFTP_buffer_send(ssh, &sftpBuf);
+    ret = wolfSSH_TestSftpBufferSend(ssh, testData, sizeof(testData), 0);
     AssertIntEQ(ret, WS_WANT_WRITE);
 
-    /* Verify the SFTP buffer was NOT consumed (idx should still be 0).
-     * This is critical - the SFTP layer must not advance its buffer index
-     * until the SSH output buffer is flushed. */
-    AssertIntEQ(sftpBuf.idx, 0);
-
     /* Verify the SSH output buffer still has pending data */
     AssertTrue(ssh->outputBuffer.length > ssh->outputBuffer.idx);
 

wolfssh/internal.h

@@ -1072,6 +1072,7 @@ enum ChannelOpenFailReasons {
 WOLFSSH_LOCAL int DoReceive(WOLFSSH*);
 WOLFSSH_LOCAL int DoProtoId(WOLFSSH*);
 WOLFSSH_LOCAL int wolfSSH_SendPacket(WOLFSSH*);
+WOLFSSH_LOCAL int wolfSSH_OutputPending(WOLFSSH*);
 WOLFSSH_LOCAL int SendProtoId(WOLFSSH*);
 WOLFSSH_LOCAL int SendKexInit(WOLFSSH*);
 WOLFSSH_LOCAL int SendKexDhInit(WOLFSSH*);

wolfssh/wolfsftp.h

@@ -173,13 +173,6 @@ struct WS_SFTPNAME {
     #define WOLFSSH_MAX_SFTP_RECV 32768
 #endif
 
-/* SFTP buffer for nonblocking state tracking */
-typedef struct WS_SFTP_BUFFER {
-    byte*  data;
-    word32 sz;
-    word32 idx;
-} WS_SFTP_BUFFER;
-
 /* functions for establishing a connection */
 WOLFSSH_API int wolfSSH_SFTP_accept(WOLFSSH* ssh);
 WOLFSSH_API int wolfSSH_SFTP_connect(WOLFSSH* ssh);
@@ -289,8 +282,10 @@ WOLFSSL_LOCAL int SFTP_RemoveHandleNode(WOLFSSH* ssh, byte* handle,
 
 WOLFSSH_LOCAL void wolfSSH_SFTP_ShowSizes(void);
 
-/* SFTP buffer send - exposed for testing */
-WOLFSSH_LOCAL int wolfSSH_SFTP_buffer_send(WOLFSSH* ssh, WS_SFTP_BUFFER* buffer);
+#ifdef WOLFSSH_TEST_INTERNAL
+    WOLFSSH_API int wolfSSH_TestSftpBufferSend(WOLFSSH* ssh,
+            byte* data, word32 sz, word32 idx);
+#endif
 
 #ifdef __cplusplus
 }