GUACAMOLE-2180: Improve SFTP upload performance

src/common-ssh/sftp.c

@@ -34,6 +34,148 @@
 #include <stdlib.h>
 #include <string.h>
 
+/*
+ * Size of the per-upload buffer, in bytes.
+ *
+ * SFTP packets carry up to 32KB of payload, but libssh2_sftp_write() can
+ * accept larger buffers and will internally split them across multiple SFTP
+ * packets.
+ */
+#define GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE (256 * 1024)
+
+/**
+ * Internal state tracking structure for an in-progress SFTP file upload.
+ *
+ * This structure manages the buffered write strategy for SFTP uploads,
+ * accumulating incoming blob data in a local buffer before flushing to the
+ * remote server. This approach reduces the number of SFTP round-trips and
+ * improves upload throughput, particularly for high-latency connections.
+ *
+ * The upload lifecycle is as follows:
+ *   1. Allocated when a file stream is initiated via
+ *      guac_common_ssh_sftp_handle_file_stream().
+ *   2. Populated incrementally by guac_common_ssh_sftp_blob_handler() as
+ *      blob messages arrive from the Guacamole client.
+ *   3. Flushed to the remote file via guac_common_ssh_sftp_upload_flush()
+ *      when the buffer is full or the stream ends.
+ *   4. Freed by guac_common_ssh_sftp_end_handler() when the upload completes.
+ *
+ * @see guac_common_ssh_sftp_handle_file_stream()
+ * @see guac_common_ssh_sftp_blob_handler()
+ * @see guac_common_ssh_sftp_upload_flush()
+ * @see guac_common_ssh_sftp_end_handler()
+ */
+typedef struct guac_common_ssh_sftp_upload_state {
+
+    /**
+     * The underlying libssh2 SFTP file handle for the remote file being
+     * written. This handle is opened when the upload begins and closed
+     * when the stream ends. May be NULL if the file failed to open.
+     */
+    LIBSSH2_SFTP_HANDLE* file;
+
+    /**
+     * Local buffer for accumulating upload data before writing to the remote
+     * file. Data from incoming Guacamole blob messages is copied here until
+     * the buffer is full or the upload ends, at which point it is flushed
+     * via libssh2_sftp_write(). The buffer size is defined by
+     * GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE.
+     */
+    char buffer[GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE];
+
+    /**
+     * The number of bytes currently stored in the buffer awaiting flush.
+     * This value ranges from 0 to GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE.
+     * Reset to 0 after each successful flush operation.
+     */
+    int buffered_length;
+
+    /**
+     * Error flag indicating whether a write failure has occurred during
+     * this upload. Once set to non-zero, subsequent blob handlers will
+     * immediately report failure without attempting further writes.
+     * A value of 0 indicates no error; non-zero indicates failure.
+     */
+    int error;
+
+} guac_common_ssh_sftp_upload_state;
+
+/**
+ * Flushes any buffered upload data to the remote SFTP file.
+ *
+ * Partial writes are retried until complete or an error occurs. On error, the
+ * upload state is marked so that subsequent blobs can report failure.
+ *
+ * NOTE: libssh2_sftp_write() may return a short count even when no error has
+ * occurred. A short return is still progress. Continue calling until the
+ * entire buffer has been consumed before reusing or modifying it.
+ *
+ * @param user
+ *     The user receiving the upload.
+ *
+ * @param state
+ *     Per-stream upload state containing the SFTP handle and buffered data.
+ *
+ * @return
+ *     Nothing.
+ */
+static void guac_common_ssh_sftp_upload_flush(guac_user* user,
+    guac_common_ssh_sftp_upload_state* state) {
+
+    /*
+     * Guard: Skip flush if any of the following conditions are true:
+     *   - A previous write error has already occurred
+     *   - The buffer is empty (nothing to flush)
+     *   - The file handle is invalid (file never opened successfully)
+     */
+    if (state->error || state->buffered_length <= 0 || state->file == NULL)
+        return;
+
+    /*
+     * Track our position within the buffer. We use an offset rather than
+     * modifying the buffer pointer so we can calculate remaining bytes easily.
+     */
+    int offset = 0;
+
+    /*
+     * Write loop: Continue until all buffered data has been written.
+     *
+     * libssh2_sftp_write() may perform a "short write" where fewer bytes are
+     * written than requested. This is normal behavior (not an error) and
+     * simply means we need to retry with the remaining data. We only treat
+     * a return value of 0 or negative as an actual failure.
+     */
+    while (offset < state->buffered_length) {
+
+        /* Attempt to write remaining buffered data starting at current offset */
+        ssize_t written = libssh2_sftp_write(state->file,
+                state->buffer + offset, state->buffered_length - offset);
+
+        /* Check for write failure (0 = no progress, negative = error) */
+        if (written <= 0) {
+            guac_user_log(user, GUAC_LOG_INFO,
+                    "Buffered SFTP write failed after %d bytes (attempt returned %zd)",
+                    offset, written);
+            state->error = 1;  /* Mark error to prevent further write attempts */
+            break;
+        }
+
+        /* Advance offset by the number of bytes successfully written */
+        offset += (int)written;
+    }
+
+    /* Log successful flush for debugging purposes */
+    if (!state->error)
+        guac_user_log(user, GUAC_LOG_DEBUG,
+                "Flushed %d buffered bytes to remote file", state->buffered_length);
+
+    /*
+     * Reset buffer length regardless of success or failure. On error, we don't
+     * want to retry the same data; on success, the buffer is now empty.
+     */
+    state->buffered_length = 0;
+}
+
 int guac_common_ssh_sftp_normalize_path(char* fullpath,
         const char* path) {
 
@@ -307,26 +449,69 @@ static int guac_ssh_append_path(char* fullpath, const char* path_a,
  */
 static int guac_common_ssh_sftp_blob_handler(guac_user* user,
         guac_stream* stream, void* data, int length) {
+    guac_common_ssh_sftp_upload_state* state =
+            (guac_common_ssh_sftp_upload_state*) stream->data;
 
-    /* Pull file from stream */
-    LIBSSH2_SFTP_HANDLE* file = (LIBSSH2_SFTP_HANDLE*) stream->data;
+    if (state == NULL) {
+        guac_user_log(user, GUAC_LOG_WARNING, "SFTP upload blob received with missing state");
+        guac_protocol_send_ack(user->socket, stream, "SFTP: Invalid state",
+                GUAC_PROTOCOL_STATUS_SERVER_ERROR);
+        guac_socket_flush(user->socket);
+        return 0;
+    }
 
-    /* Attempt write */
-    if (libssh2_sftp_write(file, data, length) == length) {
-        guac_user_log(user, GUAC_LOG_DEBUG, "%i bytes written", length);
-        guac_protocol_send_ack(user->socket, stream, "SFTP: OK",
-                GUAC_PROTOCOL_STATUS_SUCCESS);
+    /* If previous flush failed, report failure immediately */
+    if (state->error) {
+        guac_user_log(user, GUAC_LOG_WARNING, "SFTP upload blob received after previous write failure");
+        guac_protocol_send_ack(user->socket, stream, "SFTP: Previous write failed",
+                GUAC_PROTOCOL_STATUS_SERVER_ERROR);
         guac_socket_flush(user->socket);
+        return 0;
+    }
+
+    const char* incoming = (const char*) data;
+    int remaining = length;
+
+    /*
+     * Copy the incoming blob into the upload buffer. We always flush when the
+     * buffer is full so that subsequent blobs start with space available and
+     * the buffer can be reused without reallocations.
+     */
+    while (remaining > 0 && !state->error) {
+        /* If the buffer is already full, flush before copying more data. */
+        if (state->buffered_length == GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE) {
+            guac_common_ssh_sftp_upload_flush(user, state);
+            /* Defensive: bail if error occurred during flush. */
+            if (state->error)
+                break;
+
+            /* Defensive: bail if flush did not reset buffered_length. */
+            if (state->buffered_length == GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE)
+                break;
+        }
+
+        /* Copy as much of the remaining blob as will fit in the buffer. */
+        int space = GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE - state->buffered_length;
+        int to_copy = remaining < space ? remaining : space;
+
+        memcpy(state->buffer + state->buffered_length, incoming, to_copy);
+        state->buffered_length += to_copy;
+        incoming += to_copy;
+        remaining -= to_copy;
     }
 
-    /* Inform of any errors */
+    /* ACK immediately after buffering (fast path) */
+    if (!state->error) {
+        guac_user_log(user, GUAC_LOG_DEBUG, "SFTP upload buffered %d bytes (ack success)", length);
+        guac_protocol_send_ack(user->socket, stream, "SFTP: OK",
+                GUAC_PROTOCOL_STATUS_SUCCESS);
+    }
     else {
-        guac_user_log(user, GUAC_LOG_INFO, "Unable to write to file");
+        guac_user_log(user, GUAC_LOG_WARNING, "SFTP upload blob failed after buffering attempt");
         guac_protocol_send_ack(user->socket, stream, "SFTP: Write failed",
                 GUAC_PROTOCOL_STATUS_SERVER_ERROR);
-        guac_socket_flush(user->socket);
     }
-
+    guac_socket_flush(user->socket);
     return 0;
 
 }
@@ -348,24 +533,47 @@ static int guac_common_ssh_sftp_blob_handler(guac_user* user,
  */
 static int guac_common_ssh_sftp_end_handler(guac_user* user,
         guac_stream* stream) {
+    guac_common_ssh_sftp_upload_state* state =
+            (guac_common_ssh_sftp_upload_state*) stream->data;
 
-    /* Pull file from stream */
-    LIBSSH2_SFTP_HANDLE* file = (LIBSSH2_SFTP_HANDLE*) stream->data;
+    if (state == NULL) {
+        guac_protocol_send_ack(user->socket, stream, "SFTP: Invalid state",
+                GUAC_PROTOCOL_STATUS_SERVER_ERROR);
+        guac_socket_flush(user->socket);
+        return 0;
+    }
+
+    /* Flush any remaining buffered data */
+    guac_common_ssh_sftp_upload_flush(user, state);
 
-    /* Attempt to close file */
-    if (libssh2_sftp_close(file) == 0) {
-        guac_user_log(user, GUAC_LOG_DEBUG, "File closed");
+    int close_ok = 0;
+    if (state->file != NULL && !state->error) {
+        if (libssh2_sftp_close(state->file) == 0) {
+            guac_user_log(user, GUAC_LOG_DEBUG, "File closed");
+            close_ok = 1;
+        }
+        else {
+            guac_user_log(user, GUAC_LOG_INFO, "Unable to close file");
+        }
+    }
+
+    if (close_ok && !state->error) {
         guac_protocol_send_ack(user->socket, stream, "SFTP: OK",
                 GUAC_PROTOCOL_STATUS_SUCCESS);
-        guac_socket_flush(user->socket);
+    }
+    else if (state->error) {
+        guac_protocol_send_ack(user->socket, stream, "SFTP: Write failed (flush)",
+                GUAC_PROTOCOL_STATUS_SERVER_ERROR);
     }
     else {
-        guac_user_log(user, GUAC_LOG_INFO, "Unable to close file");
         guac_protocol_send_ack(user->socket, stream, "SFTP: Close failed",
                 GUAC_PROTOCOL_STATUS_SERVER_ERROR);
-        guac_socket_flush(user->socket);
     }
+    guac_socket_flush(user->socket);
 
+    /* Free state */
+    guac_mem_free(state);
+    stream->data = NULL;
     return 0;
 
 }
@@ -434,8 +642,13 @@ int guac_common_ssh_sftp_handle_file_stream(
     stream->blob_handler = guac_common_ssh_sftp_blob_handler;
     stream->end_handler = guac_common_ssh_sftp_end_handler;
 
-    /* Store file within stream */
-    stream->data = file;
+    /* Allocate and initialize upload state */
+    guac_common_ssh_sftp_upload_state* state =
+            guac_mem_alloc(sizeof(guac_common_ssh_sftp_upload_state));
+    state->file = file;
+    state->buffered_length = 0;
+    state->error = (file == NULL) ? 1 : 0; /* Mark error if file open failed */
+    stream->data = state;
     return 0;
 
 }
@@ -917,8 +1130,13 @@ static int guac_common_ssh_sftp_put_handler(guac_user* user,
     stream->blob_handler = guac_common_ssh_sftp_blob_handler;
     stream->end_handler = guac_common_ssh_sftp_end_handler;
 
-    /* Store file within stream */
-    stream->data = file;
+    /* Allocate and initialize upload state */
+    guac_common_ssh_sftp_upload_state* state =
+            guac_mem_alloc(sizeof(guac_common_ssh_sftp_upload_state));
+    state->file = file;
+    state->buffered_length = 0;
+    state->error = (file == NULL) ? 1 : 0;
+    stream->data = state;
 
     guac_socket_flush(user->socket);
     return 0;