Repository: guacamole-server Updated Branches: refs/heads/master aba7b987d -> bb9560716
GUACAMOLE-662: Correct handling of buffering within nested socket. The nested socket implementation seems to have never been properly updated since guac_socket was changed to rely on implementation-specific buffering. This meant that absolutely every write resulted in a nest instruction being sent to the parent socket. Data should instead be built up within the internal buffer, with each flush writing as much of the internal buffer as possible within a nest instruction, leaving any partial UTF-8 characters at the end of the buffer for later completion with future writes. Project: http://git-wip-us.apache.org/repos/asf/guacamole-server/repo Commit: http://git-wip-us.apache.org/repos/asf/guacamole-server/commit/cc4671d7 Tree: http://git-wip-us.apache.org/repos/asf/guacamole-server/tree/cc4671d7 Diff: http://git-wip-us.apache.org/repos/asf/guacamole-server/diff/cc4671d7 Branch: refs/heads/master Commit: cc4671d7a168cd7424282f9b4be7b59cc1ee36a2 Parents: 47ad6f4 Author: Michael Jumper <mjum...@apache.org> Authored: Sun Jan 6 17:06:45 2019 -0800 Committer: Michael Jumper <mjum...@apache.org> Committed: Sun Jan 6 17:09:35 2019 -0800 ---------------------------------------------------------------------- src/libguac/socket-nest.c | 292 ++++++++++++++++++++++++++++++++++------- 1 file changed, 241 insertions(+), 51 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/guacamole-server/blob/cc4671d7/src/libguac/socket-nest.c ---------------------------------------------------------------------- diff --git a/src/libguac/socket-nest.c b/src/libguac/socket-nest.c index 0c7d471..967c9d9 100644 --- a/src/libguac/socket-nest.c +++ b/src/libguac/socket-nest.c @@ -25,80 +25,237 @@ #include <stddef.h> #include <stdlib.h> +#include <string.h> #include <unistd.h> -#define GUAC_SOCKET_NEST_BUFFER_SIZE 8192 +/** + * The maximum number of bytes to buffer before sending a "nest" instruction. + * As some of the 8 KB space available for each instruction will be taken up by + * the "nest" opcode and other parameters, and 1 KB will be more than enough + * space for that extra data, this space is reduced to an even 7 KB. + */ +#define GUAC_SOCKET_NEST_BUFFER_SIZE 7168 -typedef struct __guac_socket_nest_data { +/** + * Internal data associated with an open socket which writes via a series of + * "nest" instructions to some underlying, parent socket. + */ +typedef struct guac_socket_nest_data { + /** + * The underlying socket which should be used to write "nest" instructions. + */ guac_socket* parent; - char buffer[GUAC_SOCKET_NEST_BUFFER_SIZE]; + + /** + * The arbitrary index of the nested socket, assigned at time of + * allocation. + */ int index; -} __guac_socket_nest_data; + /** + * The number of bytes currently in the main write buffer. + */ + int written; -ssize_t __guac_socket_nest_write_handler(guac_socket* socket, - const void* buf, size_t count) { + /** + * The main write buffer. Bytes written go here before being flushed + * as nest instructions. Space is included for the null terminator + * required by guac_protocol_send_nest(). + */ + char buffer[GUAC_SOCKET_NEST_BUFFER_SIZE]; + + /** + * Lock which is acquired when an instruction is being written, and + * released when the instruction is finished being written. + */ + pthread_mutex_t socket_lock; + + /** + * Lock which protects access to the internal buffer of this socket, + * guaranteeing atomicity of writes and flushes. + */ + pthread_mutex_t buffer_lock; + +} guac_socket_nest_data; + +/** + * Flushes the contents of the output buffer of the given socket immediately, + * without first locking access to the output buffer. This function must ONLY + * be called if the buffer lock has already been acquired. + * + * @param socket + * The guac_socket to flush. + * + * @return + * Zero if the flush operation was successful, non-zero otherwise. + */ +static ssize_t guac_socket_nest_flush(guac_socket* socket) { + + guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data; + + /* Flush remaining bytes in buffer */ + if (data->written > 0) { + + /* Determine length of buffer containing complete UTF-8 characters + * (buffer may end with a partial, multi-byte character) */ + int length = 0; + while (length < data->written) + length += guac_utf8_charsize(*(data->buffer + length)); - __guac_socket_nest_data* data = (__guac_socket_nest_data*) socket->data; - unsigned char* source = (unsigned char*) buf; + /* Add null terminator, preserving overwritten character for later + * restoration (guac_protocol_send_nest() requires null-terminated + * strings) */ + char overwritten = data->buffer[length]; + data->buffer[length] = '\0'; - /* Current location in destination buffer during copy */ - char* current = data->buffer; + /* Write ALL bytes in buffer as nest instruction */ + int retval = guac_protocol_send_nest(data->parent, data->index, data->buffer); - /* Number of bytes remaining in source buffer */ - int remaining = count; + /* Restore original value overwritten by null terminator */ + data->buffer[length] = overwritten; - /* If we can't actually store that many bytes, reduce number of bytes - * expected to be written */ - if (remaining > GUAC_SOCKET_NEST_BUFFER_SIZE) - remaining = GUAC_SOCKET_NEST_BUFFER_SIZE; + if (retval) + return 1; + + /* Shift any remaining data to beginning of buffer */ + memcpy(data->buffer, data->buffer + length, data->written - length); + data->written -= length; + + } + + return 0; + +} + +/** + * Flushes the internal buffer of the given guac_socket, writing all data + * to the underlying socket using "nest" instructions. + * + * @param socket + * The guac_socket to flush. + * + * @return + * Zero if the flush operation was successful, non-zero otherwise. + */ +static ssize_t guac_socket_nest_flush_handler(guac_socket* socket) { + + int retval; + guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data; + + /* Acquire exclusive access to buffer */ + pthread_mutex_lock(&(data->buffer_lock)); + + /* Flush contents of buffer */ + retval = guac_socket_nest_flush(socket); + + /* Relinquish exclusive access to buffer */ + pthread_mutex_unlock(&(data->buffer_lock)); + + return retval; + +} - /* Current offset within destination buffer */ - int offset; +/** + * Writes the contents of the buffer to the output buffer of the given socket, + * flushing the output buffer as necessary, without first locking access to the + * output buffer. This function must ONLY be called if the buffer lock has + * already been acquired. + * + * @param socket + * The guac_socket to write the given buffer to. + * + * @param buf + * The buffer to write to the given socket. + * + * @param count + * The number of bytes in the given buffer. + * + * @return + * The number of bytes written, or a negative value if an error occurs + * during write. + */ +static ssize_t guac_socket_nest_write_buffered(guac_socket* socket, + const void* buf, size_t count) { - /* Number of characters before start of next character */ - int skip = 0; + size_t original_count = count; + const char* current = buf; + guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data; - /* Copy UTF-8 characters into buffer */ - for (offset = 0; offset < GUAC_SOCKET_NEST_BUFFER_SIZE; offset++) { + /* Append to buffer, flush if necessary */ + while (count > 0) { - /* Get next byte */ - unsigned char c = *source; - remaining--; + int chunk_size; - /* If skipping, then skip */ - if (skip > 0) skip--; + /* Calculate space remaining, including one extra byte for the null + * terminator added upon flush */ + int remaining = sizeof(data->buffer) - data->written - 1; - /* Otherwise, determine next skip value, and increment length */ - else { + /* If no space left in buffer, flush and retry */ + if (remaining == 0) { - /* Determine skip value (size in bytes of rest of character) */ - skip = guac_utf8_charsize(c) - 1; + /* Abort if error occurs during flush */ + if (guac_socket_nest_flush(socket)) + return -1; - /* If not enough bytes to complete character, break */ - if (skip > remaining) - break; + /* Retry buffer append */ + continue; } - /* Store byte */ - *current = c; + /* Calculate size of chunk to be written to buffer */ + chunk_size = count; + if (chunk_size > remaining) + chunk_size = remaining; - /* Advance to next character */ - source++; - current++; + /* Update output buffer */ + memcpy(data->buffer + data->written, current, chunk_size); + data->written += chunk_size; + + /* Update provided buffer */ + current += chunk_size; + count -= chunk_size; } - /* Append null-terminator */ - *current = 0; + /* All bytes have been written, possibly some to the internal buffer */ + return original_count; + +} + +/** + * Appends the provided data to the internal buffer for future writing. The + * actual write attempt will occur only upon flush, or when the internal buffer + * is full. + * + * @param socket + * The guac_socket being write to. + * + * @param buf + * The arbitrary buffer containing the data to be written. + * + * @param count + * The number of bytes contained within the buffer. + * + * @return + * The number of bytes written, or -1 if an error occurs. + */ +static ssize_t guac_socket_nest_write_handler(guac_socket* socket, + const void* buf, size_t count) { + + int retval; + guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data; + + /* Acquire exclusive access to buffer */ + pthread_mutex_lock(&(data->buffer_lock)); + + /* Write provided data to buffer */ + retval = guac_socket_nest_write_buffered(socket, buf, count); - /* Send nest instruction containing read UTF-8 segment */ - guac_protocol_send_nest(data->parent, data->index, data->buffer); + /* Relinquish exclusive access to buffer */ + pthread_mutex_unlock(&(data->buffer_lock)); - /* Return number of bytes actually written */ - return offset; + return retval; } @@ -113,30 +270,63 @@ ssize_t __guac_socket_nest_write_handler(guac_socket* socket, * Zero if the data was successfully freed, non-zero otherwise. This * implementation always succeeds, and will always return zero. */ -static int __guac_socket_nest_free_handler(guac_socket* socket) { +static int guac_socket_nest_free_handler(guac_socket* socket) { /* Free associated data */ - __guac_socket_nest_data* data = (__guac_socket_nest_data*) socket->data; + guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data; free(data); return 0; } +/** + * Acquires exclusive access to the given socket. + * + * @param socket + * The guac_socket to which exclusive access is required. + */ +static void guac_socket_nest_lock_handler(guac_socket* socket) { + + guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data; + + /* Acquire exclusive access to socket */ + pthread_mutex_lock(&(data->socket_lock)); + +} + +/** + * Relinquishes exclusive access to the given socket. + * + * @param socket + * The guac_socket to which exclusive access is no longer required. + */ +static void guac_socket_nest_unlock_handler(guac_socket* socket) { + + guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data; + + /* Relinquish exclusive access to socket */ + pthread_mutex_unlock(&(data->socket_lock)); + +} + guac_socket* guac_socket_nest(guac_socket* parent, int index) { /* Allocate socket and associated data */ guac_socket* socket = guac_socket_alloc(); - __guac_socket_nest_data* data = malloc(sizeof(__guac_socket_nest_data)); + guac_socket_nest_data* data = malloc(sizeof(guac_socket_nest_data)); /* Store nested socket details as socket data */ data->parent = parent; data->index = index; socket->data = data; - /* Set write and free handlers */ - socket->write_handler = __guac_socket_nest_write_handler; - socket->free_handler = __guac_socket_nest_free_handler; + /* Set relevant handlers */ + socket->write_handler = guac_socket_nest_write_handler; + socket->lock_handler = guac_socket_nest_lock_handler; + socket->unlock_handler = guac_socket_nest_unlock_handler; + socket->flush_handler = guac_socket_nest_flush_handler; + socket->free_handler = guac_socket_nest_free_handler; return socket;
