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.

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];
 
-    __guac_socket_nest_data* data = (__guac_socket_nest_data*) socket->data;
+    /**
-    unsigned char* source = (unsigned char*) buf;
+     * Lock which is acquired when an instruction is being written, and
+     * released when the instruction is finished being written.
+     */
+    pthread_mutex_t socket_lock;
 
-    /* Current location in destination buffer during copy */
+    /**
-    char* current = data->buffer;
+     * Lock which protects access to the internal buffer of this socket,
+     * guaranteeing atomicity of writes and flushes.
+     */
+    pthread_mutex_t buffer_lock;
 
-    /* Number of bytes remaining in source buffer */
+} guac_socket_nest_data;
-    int remaining = count;
 
-    /* If we can't actually store that many bytes, reduce number of bytes
+/**
-     * expected to be written */
+ * Flushes the contents of the output buffer of the given socket immediately,
-    if (remaining > GUAC_SOCKET_NEST_BUFFER_SIZE)
+ * without first locking access to the output buffer. This function must ONLY
-        remaining = GUAC_SOCKET_NEST_BUFFER_SIZE;
+ * 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) {
 
-    /* Current offset within destination buffer */
+    guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data;
-    int offset;
 
-    /* Number of characters before start of next character */
+    /* Flush remaining bytes in buffer */
-    int skip = 0;
+    if (data->written > 0) {
 
-    /* Copy UTF-8 characters into buffer */
+        /* Determine length of buffer containing complete UTF-8 characters
-    for (offset = 0; offset < GUAC_SOCKET_NEST_BUFFER_SIZE; offset++) {
+         * (buffer may end with a partial, multi-byte character) */
+        int length = 0;
+        while (length < data->written)
+            length += guac_utf8_charsize(*(data->buffer + length));
 
-        /* Get next byte */
+        /* Add null terminator, preserving overwritten character for later
-        unsigned char c = *source;
+         * restoration (guac_protocol_send_nest() requires null-terminated
-        remaining--;
+         * strings) */
+        char overwritten = data->buffer[length];
+        data->buffer[length] = '\0';
 
-        /* If skipping, then skip */
+        /* Write ALL bytes in buffer as nest instruction */
-        if (skip > 0) skip--;
+        int retval = guac_protocol_send_nest(data->parent, data->index, data->buffer);
 
-        /* Otherwise, determine next skip value, and increment length */
+        /* Restore original value overwritten by null terminator */
-        else {
+        data->buffer[length] = overwritten;
 
-            /* Determine skip value (size in bytes of rest of character) */
+        if (retval)
-            skip = guac_utf8_charsize(c) - 1;
+            return 1;
 
-            /* If not enough bytes to complete character, break */
+        /* Shift any remaining data to beginning of buffer */
-            if (skip > remaining)
+        memcpy(data->buffer, data->buffer + length, data->written - length);
-                break;
+        data->written -= length;
-
-        }
-
-        /* Store byte */
-        *current = c;
-
-        /* Advance to next character */
-        source++;
-        current++;
 
     }
 
-    /* Append null-terminator */
+    return 0;
-    *current = 0;
 
-    /* Send nest instruction containing read UTF-8 segment */
+}
-    guac_protocol_send_nest(data->parent, data->index, data->buffer);
 
-    /* Return number of bytes actually written */
+/**
-    return offset;
+ * 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;
+
+}
+
+/**
+ * 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) {
+
+    size_t original_count = count;
+    const char* current = buf;
+    guac_socket_nest_data* data = (guac_socket_nest_data*) socket->data;
+
+    /* Append to buffer, flush if necessary */
+    while (count > 0) {
+
+        int chunk_size;
+
+        /* Calculate space remaining, including one extra byte for the null
+         * terminator added upon flush */
+        int remaining = sizeof(data->buffer) - data->written - 1;
+
+        /* If no space left in buffer, flush and retry */
+        if (remaining == 0) {
+
+            /* Abort if error occurs during flush */
+            if (guac_socket_nest_flush(socket))
+                return -1;
+
+            /* Retry buffer append */
+            continue;
+
+        }
+
+        /* Calculate size of chunk to be written to buffer */
+        chunk_size = count;
+        if (chunk_size > remaining)
+            chunk_size = remaining;
+
+        /* Update output buffer */
+        memcpy(data->buffer + data->written, current, chunk_size);
+        data->written += chunk_size;
+
+        /* Update provided buffer */
+        current += chunk_size;
+        count   -= chunk_size;
+
+    }
+
+    /* 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);
+
+    /* Relinquish exclusive access to buffer */
+    pthread_mutex_unlock(&(data->buffer_lock));
+
+    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 */
+    /* Set relevant handlers */
-    socket->write_handler  = __guac_socket_nest_write_handler;
+    socket->write_handler  = guac_socket_nest_write_handler;
-    socket->free_handler   = __guac_socket_nest_free_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;