From 2706080736889d733cc5e206c5f5cb7d71f6107c Mon Sep 17 00:00:00 2001 From: alailsonko Date: Mon, 15 Dec 2025 18:59:30 +0000 Subject: [PATCH 1/2] GUACAMOLE-2180: Improve SFTP upload performance by implementing buffered writes --- src/common-ssh/sftp.c | 184 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 161 insertions(+), 23 deletions(-) diff --git a/src/common-ssh/sftp.c b/src/common-ssh/sftp.c index 4a6f7716ab..f4fc905000 100644 --- a/src/common-ssh/sftp.c +++ b/src/common-ssh/sftp.c @@ -34,6 +34,68 @@ #include #include +/* + * 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) + +typedef struct guac_common_ssh_sftp_upload_state { + LIBSSH2_SFTP_HANDLE* file; /* Underlying SFTP file handle */ + char buffer[GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE]; /* Buffered upload data */ + int buffered_length; /* Bytes currently buffered */ + int error; /* Non-zero if a write error occurred */ +} 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) { + + if (state->error || state->buffered_length <= 0 || state->file == NULL) + return; + + int offset = 0; + while (offset < state->buffered_length) { + ssize_t written = libssh2_sftp_write(state->file, + state->buffer + offset, state->buffered_length - offset); + 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; + break; + } + offset += (int)written; + } + + if (!state->error) + guac_user_log(user, GUAC_LOG_DEBUG, + "Flushed %d buffered bytes to remote file", state->buffered_length); + + state->buffered_length = 0; +} + int guac_common_ssh_sftp_normalize_path(char* fullpath, const char* path) { @@ -307,26 +369,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 +453,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 +562,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 +1050,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; From 40a37267e8e3065b564978ef2078dc0c6de55b94 Mon Sep 17 00:00:00 2001 From: alailsonko Date: Mon, 26 Jan 2026 08:33:30 +0000 Subject: [PATCH 2/2] GUACAMOLE-2180: Document SFTP upload state structure and flush function. --- src/common-ssh/sftp.c | 90 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 85 insertions(+), 5 deletions(-) diff --git a/src/common-ssh/sftp.c b/src/common-ssh/sftp.c index f4fc905000..21b62e022c 100644 --- a/src/common-ssh/sftp.c +++ b/src/common-ssh/sftp.c @@ -43,11 +43,61 @@ */ #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 { - LIBSSH2_SFTP_HANDLE* file; /* Underlying SFTP file handle */ - char buffer[GUAC_COMMON_SSH_SFTP_UPLOAD_BUFFER_SIZE]; /* Buffered upload data */ - int buffered_length; /* Bytes currently buffered */ - int error; /* Non-zero if a write error occurred */ + + /** + * 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; /** @@ -72,27 +122,57 @@ typedef struct guac_common_ssh_sftp_upload_state { 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; + 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; }