GUACAMOLE-615: Merge fix for potential parse failures in webapp parser implementations.

guacamole-common-js/src/main/webapp/modules/Parser.js

@@ -22,14 +22,16 @@ var Guacamole = Guacamole || {};
 /**
  * Simple Guacamole protocol parser that invokes an oninstruction event when
  * full instructions are available from data received via receive().
- * 
+ *
  * @constructor
  */
-Guacamole.Parser = function() {
+Guacamole.Parser = function Parser() {
 
     /**
      * Reference to this parser.
+     *
      * @private
+     * @type {!Guacamole.Parser}
      */
     var parser = this;
 
@@ -37,24 +39,74 @@ Guacamole.Parser = function() {
      * Current buffer of received data. This buffer grows until a full
      * element is available. After a full element is available, that element
      * is flushed into the element buffer.
-     * 
+     *
      * @private
+     * @type {!string}
      */
-    var buffer = "";
+    var buffer = '';
 
     /**
      * Buffer of all received, complete elements. After an entire instruction
      * is read, this buffer is flushed, and a new instruction begins.
-     * 
+     *
      * @private
+     * @type {!string[]}
      */
-    var element_buffer = [];
+    var elementBuffer = [];
 
-    // The location of the last element's terminator
+    /**
-    var element_end = -1;
+     * The character offset within the buffer of the current or most recently
+     * parsed element's terminator. If sufficient characters have not yet been
+     * read via calls to receive(), this may point to an offset well beyond the
+     * end of the buffer. If no characters for an element have yet been read,
+     * this will be -1.
+     *
+     * @private
+     * @type {!number}
+     */
+    var elementEnd = -1;
 
-    // Where to start the next length search or the next element
+    /**
-    var start_index = 0;
+     * The character offset within the buffer of the location that the parser
+     * should start looking for the next element length search or next element
+     * value.
+     *
+     * @private
+     * @type {!number}
+     */
+    var startIndex = 0;
+
+    /**
+     * The declared length of the current element being parsed, in Unicode
+     * codepoints.
+     *
+     * @private
+     * @type {!number}
+     */
+    var elementCodepoints = 0;
+
+    /**
+     * The number of parsed characters that must accumulate in the begining of
+     * the parse buffer before processing time is expended to truncate that
+     * buffer and conserve memory.
+     *
+     * @private
+     * @constant
+     * @type {!number}
+     */
+    var BUFFER_TRUNCATION_THRESHOLD = 4096;
+
+    /**
+     * The lowest Unicode codepoint to require a surrogate pair when encoded
+     * with UTF-16. In UTF-16, characters with codepoints at or above this
+     * value are represented with a surrogate pair, while characters with
+     * codepoints below this value are represented with a single character.
+     *
+     * @private
+     * @constant
+     * @type {!number}
+     */
+    var MIN_CODEPOINT_REQUIRES_SURROGATE = 0x10000;
 
     /**
      * Appends the given instruction data packet to the internal buffer of
@@ -63,80 +115,135 @@ Guacamole.Parser = function() {
      *
      * @param {!string} packet
      *     The instruction data to receive.
+     *
+     * @param {!boolean} [isBuffer=false]
+     *     Whether the provided data should be treated as an instruction buffer
+     *     that grows continuously. If true, the data provided to receive()
+     *     MUST always start with the data provided to the previous call. If
+     *     false (the default), only the new data should be provided to
+     *     receive(), and previously-received data will automatically be
+     *     buffered by the parser as needed.
      */
-    this.receive = function(packet) {
+    this.receive = function receive(packet, isBuffer) {
 
-        // Truncate buffer as necessary
+        if (isBuffer)
-        if (start_index > 4096 && element_end >= start_index) {
+            buffer = packet;
 
-            buffer = buffer.substring(start_index);
+        else {
 
-            // Reset parse relative to truncation
+            // Truncate buffer as necessary
-            element_end -= start_index;
+            if (startIndex > BUFFER_TRUNCATION_THRESHOLD && elementEnd >= startIndex) {
-            start_index = 0;
+
+                buffer = buffer.substring(startIndex);
+
+                // Reset parse relative to truncation
+                elementEnd -= startIndex;
+                startIndex = 0;
+
+            }
+
+            // Append data to buffer ONLY if there is outstanding data present. It
+            // is otherwise much faster to simply parse the received buffer as-is,
+            // and tunnel implementations can take advantage of this by preferring
+            // to send only complete instructions. Both the HTTP and WebSocket
+            // tunnel implementations included with Guacamole already do this.
+            if (buffer.length)
+                buffer += packet;
+            else
+                buffer = packet;
 
         }
 
-        // Append data to buffer
-        buffer += packet;
-
         // While search is within currently received data
-        while (element_end < buffer.length) {
+        while (elementEnd < buffer.length) {
 
             // If we are waiting for element data
-            if (element_end >= start_index) {
+            if (elementEnd >= startIndex) {
+
+                // If we have enough data in the buffer to fill the element
+                // value, but the number of codepoints in the expected substring
+                // containing the element value value is less that its declared
+                // length, that can only be because the element contains
+                // characters split between high and low surrogates, and the
+                // actual end of the element value is further out. The minimum
+                // number of additional characters that must be read to satisfy
+                // the declared length is simply the difference between the
+                // number of codepoints actually present vs. the expected
+                // length.
+                var codepoints = Guacamole.Parser.codePointCount(buffer, startIndex, elementEnd);
+                if (codepoints < elementCodepoints) {
+                    elementEnd += elementCodepoints - codepoints;
+                    continue;
+                }
+
+                // If the current element ends with a character involving both
+                // a high and low surrogate, elementEnd points to the low
+                // surrogate and NOT the element terminator. We must shift the
+                // end and reevaluate.
+                else if (elementCodepoints && buffer.codePointAt(elementEnd - 1) >= MIN_CODEPOINT_REQUIRES_SURROGATE) {
+                    elementEnd++;
+                    continue;
+                }
 
                 // We now have enough data for the element. Parse.
-                var element = buffer.substring(start_index, element_end);
+                var element = buffer.substring(startIndex, elementEnd);
-                var terminator = buffer.substring(element_end, element_end+1);
+                var terminator = buffer.substring(elementEnd, elementEnd + 1);
 
                 // Add element to array
-                element_buffer.push(element);
+                elementBuffer.push(element);
 
                 // If last element, handle instruction
-                if (terminator == ";") {
+                if (terminator === ';') {
 
                     // Get opcode
-                    var opcode = element_buffer.shift();
+                    var opcode = elementBuffer.shift();
 
                     // Call instruction handler.
-                    if (parser.oninstruction != null)
+                    if (parser.oninstruction !== null)
-                        parser.oninstruction(opcode, element_buffer);
+                        parser.oninstruction(opcode, elementBuffer);
 
                     // Clear elements
-                    element_buffer.length = 0;
+                    elementBuffer = [];
+
+                    // Immediately truncate buffer if its contents have been
+                    // completely parsed, so that the next call to receive()
+                    // need not append to the buffer unnecessarily
+                    if (elementEnd + 1 === buffer.length) {
+                        elementEnd = -1;
+                        buffer = '';
+                    }
 
                 }
-                else if (terminator != ',')
+                else if (terminator !== ',')
-                    throw new Error("Illegal terminator.");
+                    throw new Error('Element terminator of instruction was not ";" nor ",".');
 
                 // Start searching for length at character after
                 // element terminator
-                start_index = element_end + 1;
+                startIndex = elementEnd + 1;
 
             }
 
             // Search for end of length
-            var length_end = buffer.indexOf(".", start_index);
+            var lengthEnd = buffer.indexOf('.', startIndex);
-            if (length_end != -1) {
+            if (lengthEnd !== -1) {
 
                 // Parse length
-                var length = parseInt(buffer.substring(element_end+1, length_end));
+                elementCodepoints = parseInt(buffer.substring(elementEnd + 1, lengthEnd));
-                if (isNaN(length))
+                if (isNaN(elementCodepoints))
-                    throw new Error("Non-numeric character in element length.");
+                    throw new Error('Non-numeric character in element length.');
 
                 // Calculate start of element
-                start_index = length_end + 1;
+                startIndex = lengthEnd + 1;
 
                 // Calculate location of element terminator
-                element_end = start_index + length;
+                elementEnd = startIndex + elementCodepoints;
 
             }
-            
+
             // If no period yet, continue search when more data
             // is received
             else {
-                start_index = buffer.length;
+                startIndex = buffer.length;
                 break;
             }
 
@@ -146,7 +253,7 @@ Guacamole.Parser = function() {
 
     /**
      * Fired once for every complete Guacamole instruction received, in order.
-     * 
+     *
      * @event
      * @param {!string} opcode
      *     The Guacamole instruction opcode.
@@ -157,3 +264,85 @@ Guacamole.Parser = function() {
     this.oninstruction = null;
 
 };
+
+/**
+ * Returns the number of Unicode codepoints (not code units) within the given
+ * string. If character offsets are provided, only codepoints between those
+ * offsets are counted. Unlike the length property of a string, this function
+ * counts proper surrogate pairs as a single codepoint. High and low surrogate
+ * characters that are not part of a proper surrogate pair are counted
+ * separately as individual codepoints.
+ *
+ * @param {!string} str
+ *     The string whose contents should be inspected.
+ *
+ * @param {number} [start=0]
+ *     The index of the location in the given string where codepoint counting
+ *     should start. If omitted, counting will begin at the start of the
+ *     string.
+ *
+ * @param {number} [end]
+ *     The index of the first location in the given string after where counting
+ *     should stop (the character after the last character being counted). If
+ *     omitted, all characters after the start location will be counted.
+ *
+ * @returns {!number}
+ *     The number of Unicode codepoints within the requested portion of the
+ *     given string.
+ */
+Guacamole.Parser.codePointCount = function codePointCount(str, start, end) {
+
+    // Count only characters within the specified region
+    str = str.substring(start || 0, end);
+
+    // Locate each proper Unicode surrogate pair (one high surrogate followed
+    // by one low surrogate)
+    var surrogatePairs = str.match(/[\uD800-\uDBFF][\uDC00-\uDFFF]/g);
+
+    // Each surrogate pair represents a single codepoint but is represented by
+    // two characters in a JavaScript string, and thus is counted twice toward
+    // string length. Subtracting the number of surrogate pairs adjusts that
+    // length value such that it gives us the number of codepoints.
+    return str.length - (surrogatePairs ? surrogatePairs.length : 0);
+
+};
+
+/**
+ * Converts each of the values within the given array to strings, formatting
+ * those strings as length-prefixed elements of a complete Guacamole
+ * instruction.
+ *
+ * @param {!*[]} elements
+ *     The values that should be encoded as the elements of a Guacamole
+ *     instruction. Order of these elements is preserved. This array MUST have
+ *     at least one element.
+ *
+ * @returns {!string}
+ *     A complete Guacamole instruction consisting of each of the provided
+ *     element values, in order.
+ */
+Guacamole.Parser.toInstruction = function toInstruction(elements) {
+
+    /**
+     * Converts the given value to a length/string pair for use as an
+     * element in a Guacamole instruction.
+     *
+     * @private
+     * @param {*} value
+     *     The value to convert.
+     *
+     * @return {!string}
+     *     The converted value.
+     */
+    var toElement = function toElement(value) {
+        var str = '' + value;
+        return Guacamole.Parser.codePointCount(str) + "." + str;
+    };
+
+    var instr = toElement(elements[0]);
+    for (var i = 1; i < elements.length; i++)
+        instr += ',' + toElement(elements[i]);
+
+    return instr + ';';
+
+};

guacamole-common-js/src/main/webapp/modules/Tunnel.js

@@ -436,37 +436,11 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) {
             return;
 
         // Do not attempt to send empty messages
-        if (arguments.length === 0)
+        if (!arguments.length)
             return;
 
-        /**
-         * Converts the given value to a length/string pair for use as an
-         * element in a Guacamole instruction.
-         * 
-         * @private
-         * @param value
-         *     The value to convert.
-         *
-         * @return {!string}
-         *     The converted value.
-         */
-        function getElement(value) {
-            var string = new String(value);
-            return string.length + "." + string; 
-        }
-
-        // Initialized message with first element
-        var message = getElement(arguments[0]);
-
-        // Append remaining elements
-        for (var i=1; i<arguments.length; i++)
-            message += "," + getElement(arguments[i]);
-
-        // Final terminator
-        message += ";";
-
         // Add message to buffer
-        outputMessageBuffer += message;
+        outputMessageBuffer += Guacamole.Parser.toInstruction(arguments);
 
         // Send if not currently sending
         if (!sendingMessages)
@@ -546,14 +520,36 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) {
 
         var dataUpdateEvents = 0;
 
-        // The location of the last element's terminator
+        var parser = new Guacamole.Parser();
-        var elementEnd = -1;
+        parser.oninstruction = function instructionReceived(opcode, args) {
 
-        // Where to start the next length search or the next element
+            // Switch to next request if end-of-stream is signalled
-        var startIndex = 0;
+            if (opcode === Guacamole.Tunnel.INTERNAL_DATA_OPCODE && args.length === 0) {
 
-        // Parsed elements
+                // Reset parser state by simply switching to an entirely new
-        var elements = new Array();
+                // parser
+                parser = new Guacamole.Parser();
+                parser.oninstruction = instructionReceived;
+
+                // Clean up interval if polling
+                if (interval)
+                    clearInterval(interval);
+
+                // Clean up object
+                xmlhttprequest.onreadystatechange = null;
+                xmlhttprequest.abort();
+
+                // Start handling next request
+                if (nextRequest)
+                    handleResponse(nextRequest);
+
+            }
+
+            // Call instruction handler.
+            else if (opcode !== Guacamole.Tunnel.INTERNAL_DATA_OPCODE && tunnel.oninstruction)
+                tunnel.oninstruction(opcode, args);
+
+        };
 
         function parseResponse() {
 
@@ -614,83 +610,13 @@ Guacamole.HTTPTunnel = function(tunnelURL, crossDomain, extraTunnelHeaders) {
                 // Do not attempt to parse if data could not be read
                 catch (e) { return; }
 
-                // While search is within currently received data
+                try {
-                while (elementEnd < current.length) {
+                    parser.receive(current, true);
-
+                }
-                    // If we are waiting for element data
+                catch (e) {
-                    if (elementEnd >= startIndex) {
+                    close_tunnel(new Guacamole.Status(Guacamole.Status.Code.SERVER_ERROR, e.message));
-
+                    return;
-                        // We now have enough data for the element. Parse.
+                }
-                        var element = current.substring(startIndex, elementEnd);
-                        var terminator = current.substring(elementEnd, elementEnd+1);
-
-                        // Add element to array
-                        elements.push(element);
-
-                        // If last element, handle instruction
-                        if (terminator === ";") {
-
-                            // Get opcode
-                            var opcode = elements.shift();
-
-                            // Call instruction handler.
-                            if (tunnel.oninstruction)
-                                tunnel.oninstruction(opcode, elements);
-
-                            // Clear elements
-                            elements.length = 0;
-
-                        }
-
-                        // Start searching for length at character after
-                        // element terminator
-                        startIndex = elementEnd + 1;
-
-                    }
-
-                    // Search for end of length
-                    var lengthEnd = current.indexOf(".", startIndex);
-                    if (lengthEnd !== -1) {
-
-                        // Parse length
-                        var length = parseInt(current.substring(elementEnd+1, lengthEnd));
-
-                        // If we're done parsing, handle the next response.
-                        if (length === 0) {
-
-                            // Clean up interval if polling
-                            if (interval)
-                                clearInterval(interval);
-                           
-                            // Clean up object
-                            xmlhttprequest.onreadystatechange = null;
-                            xmlhttprequest.abort();
-
-                            // Start handling next request
-                            if (nextRequest)
-                                handleResponse(nextRequest);
-
-                            // Done parsing
-                            break;
-
-                        }
-
-                        // Calculate start of element
-                        startIndex = lengthEnd + 1;
-
-                        // Calculate location of element terminator
-                        elementEnd = startIndex + length;
-
-                    }
-                    
-                    // If no period yet, continue search when more data
-                    // is received
-                    else {
-                        startIndex = current.length;
-                        break;
-                    }
-
-                } // end parse loop
 
             }
 
@@ -823,6 +749,16 @@ Guacamole.WebSocketTunnel = function(tunnelURL) {
      */
     var tunnel = this;
 
+    /**
+     * The parser that this tunnel will use to parse received Guacamole
+     * instructions. The parser is created when the tunnel is (re-)connected.
+     * Initially, this will be null.
+     *
+     * @private
+     * @type {Guacamole.Parser}
+     */
+    var parser = null;
+
     /**
      * The WebSocket used by this tunnel.
      * 
@@ -1016,36 +952,10 @@ Guacamole.WebSocketTunnel = function(tunnelURL) {
             return;
 
         // Do not attempt to send empty messages
-        if (arguments.length === 0)
+        if (!arguments.length)
             return;
 
-        /**
+        socket.send(Guacamole.Parser.toInstruction(arguments));
-         * Converts the given value to a length/string pair for use as an
-         * element in a Guacamole instruction.
-         * 
-         * @private
-         * @param {*} value
-         *     The value to convert.
-         *
-         * @return {!string}
-         *     The converted value.
-         */
-        function getElement(value) {
-            var string = new String(value);
-            return string.length + "." + string; 
-        }
-
-        // Initialized message with first element
-        var message = getElement(arguments[0]);
-
-        // Append remaining elements
-        for (var i=1; i<arguments.length; i++)
-            message += "," + getElement(arguments[i]);
-
-        // Final terminator
-        message += ";";
-
-        socket.send(message);
 
     };
 
@@ -1056,6 +966,27 @@ Guacamole.WebSocketTunnel = function(tunnelURL) {
         // Mark the tunnel as connecting
         tunnel.setState(Guacamole.Tunnel.State.CONNECTING);
 
+        parser = new Guacamole.Parser();
+        parser.oninstruction = function instructionReceived(opcode, args) {
+
+            // Update state and UUID when first instruction received
+            if (tunnel.uuid === null) {
+
+                // Associate tunnel UUID if received
+                if (opcode === Guacamole.Tunnel.INTERNAL_DATA_OPCODE && args.length === 1)
+                    tunnel.setUUID(args[0]);
+
+                // Tunnel is now open and UUID is available
+                tunnel.setState(Guacamole.Tunnel.State.OPEN);
+
+            }
+
+            // Call instruction handler.
+            if (opcode !== Guacamole.Tunnel.INTERNAL_DATA_OPCODE && tunnel.oninstruction)
+                tunnel.oninstruction(opcode, args);
+
+        };
+
         // Connect socket
         socket = new WebSocket(tunnelURL + "?" + data, "guacamole");
 
@@ -1084,72 +1015,12 @@ Guacamole.WebSocketTunnel = function(tunnelURL) {
 
             resetTimers();
 
-            var message = event.data;
+            try {
-            var startIndex = 0;
+                parser.receive(event.data);
-            var elementEnd;
+            }
-
+            catch (e) {
-            var elements = [];
+                close_tunnel(new Guacamole.Status(Guacamole.Status.Code.SERVER_ERROR, e.message));
-
+            }
-            do {
-
-                // Search for end of length
-                var lengthEnd = message.indexOf(".", startIndex);
-                if (lengthEnd !== -1) {
-
-                    // Parse length
-                    var length = parseInt(message.substring(elementEnd+1, lengthEnd));
-
-                    // Calculate start of element
-                    startIndex = lengthEnd + 1;
-
-                    // Calculate location of element terminator
-                    elementEnd = startIndex + length;
-
-                }
-                
-                // If no period, incomplete instruction.
-                else
-                    close_tunnel(new Guacamole.Status(Guacamole.Status.Code.SERVER_ERROR, "Incomplete instruction."));
-
-                // We now have enough data for the element. Parse.
-                var element = message.substring(startIndex, elementEnd);
-                var terminator = message.substring(elementEnd, elementEnd+1);
-
-                // Add element to array
-                elements.push(element);
-
-                // If last element, handle instruction
-                if (terminator === ";") {
-
-                    // Get opcode
-                    var opcode = elements.shift();
-
-                    // Update state and UUID when first instruction received
-                    if (tunnel.uuid === null) {
-
-                        // Associate tunnel UUID if received
-                        if (opcode === Guacamole.Tunnel.INTERNAL_DATA_OPCODE && elements.length === 1)
-                            tunnel.setUUID(elements[0]);
-
-                        // Tunnel is now open and UUID is available
-                        tunnel.setState(Guacamole.Tunnel.State.OPEN);
-
-                    }
-
-                    // Call instruction handler.
-                    if (opcode !== Guacamole.Tunnel.INTERNAL_DATA_OPCODE && tunnel.oninstruction)
-                        tunnel.oninstruction(opcode, elements);
-
-                    // Clear elements
-                    elements.length = 0;
-
-                }
-
-                // Start searching for length at character after
-                // element terminator
-                startIndex = elementEnd + 1;
-
-            } while (startIndex < message.length);
 
         };
 

guacamole-common-js/src/test/javascript/ParserSpec.js

@@ -0,0 +1,221 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/* global Guacamole, jasmine, expect */
+
+describe('Guacamole.Parser', function ParserSpec() {
+
+    /**
+     * A single Unicode high surrogate character (any character between U+D800
+     * and U+DB7F).
+     *
+     * @constant
+     * @type {!string}
+     */
+    const HIGH_SURROGATE = '\uD802';
+
+    /**
+     * A single Unicode low surrogate character (any character between U+DC00
+     * and U+DFFF).
+     *
+     * @constant
+     * @type {!string}
+     */
+    const LOW_SURROGATE = '\uDF00';
+
+    /**
+     * A Unicode surrogate pair, consisting of a high and low surrogate.
+     *
+     * @constant
+     * @type {!string}
+     */
+    const SURROGATE_PAIR = HIGH_SURROGATE + LOW_SURROGATE;
+
+    /**
+     * A 4-character test string containing Unicode characters that require
+     * multiple bytes when encoded as UTF-8, including at least one character
+     * that is encoded as a surrogate pair in UTF-16.
+     *
+     * @constant
+     * @type {!string}
+     */
+    const UTF8_MULTIBYTE = '\u72AC' + SURROGATE_PAIR + 'z\u00C1';
+
+    /**
+     * The Guacamole.Parser instance to test. This instance is (re)created prior
+     * to each test via beforeEach().
+     *
+     * @type {Guacamole.Parser}
+     */
+    var parser;
+
+    // Provide each test with a fresh parser
+    beforeEach(function() {
+        parser = new Guacamole.Parser();
+    });
+
+    // Empty instruction
+    describe('when an empty instruction is received', function() {
+
+        it('should parse the single empty opcode and invoke oninstruction', function() {
+            parser.oninstruction = jasmine.createSpy('oninstruction');
+            parser.receive('0.;');
+            expect(parser.oninstruction).toHaveBeenCalledOnceWith('', [ ]);
+        });
+
+    });
+
+    // Instruction using basic Latin characters
+    describe('when an instruction is containing only basic Latin characters', function() {
+
+        it('should correctly parse each element and invoke oninstruction', function() {
+            parser.oninstruction = jasmine.createSpy('oninstruction');
+            parser.receive('5.test2,'
+                + '10.hellohello,'
+                + '15.worldworldworld;'
+            );
+            expect(parser.oninstruction).toHaveBeenCalledOnceWith('test2', [
+                'hellohello',
+                'worldworldworld'
+            ]);
+        });
+
+    });
+
+    // Instruction using characters requiring multiple bytes in UTF-8 and
+    // surrogate pairs in UTF-16, including an element ending with a surrogate
+    // pair
+    describe('when an instruction is received containing elements that '
+           + 'contain characters involving surrogate pairs', function() {
+
+        it('should correctly parse each element and invoke oninstruction', function() {
+            parser.oninstruction = jasmine.createSpy('oninstruction');
+            parser.receive('4.test,'
+                + '6.a' + UTF8_MULTIBYTE + 'b,'
+                + '5.1234' + SURROGATE_PAIR + ','
+                + '10.a' + UTF8_MULTIBYTE + UTF8_MULTIBYTE + 'c;'
+            );
+            expect(parser.oninstruction).toHaveBeenCalledOnceWith('test', [
+                'a' + UTF8_MULTIBYTE + 'b',
+                '1234' + SURROGATE_PAIR,
+                'a' + UTF8_MULTIBYTE + UTF8_MULTIBYTE + 'c'
+            ]);
+        });
+
+    });
+
+    // Instruction with an element values ending with an incomplete surrogate
+    // pair (high or low surrogate only)
+    describe('when an instruction is received containing elements that end '
+           + 'with incomplete surrogate pairs', function() {
+
+        it('should correctly parse each element and invoke oninstruction', function() {
+            parser.oninstruction = jasmine.createSpy('oninstruction');
+            parser.receive('4.test,'
+                + '5.1234' + HIGH_SURROGATE + ','
+                + '5.4567' + LOW_SURROGATE + ';'
+            );
+            expect(parser.oninstruction).toHaveBeenCalledOnceWith('test', [
+                '1234' + HIGH_SURROGATE,
+                '4567' + LOW_SURROGATE
+            ]);
+        });
+
+    });
+
+    // Instruction with element values containing incomplete surrogate pairs,
+    describe('when an instruction is received containing incomplete surrogate pairs', function() {
+
+        it('should correctly parse each element and invoke oninstruction', function() {
+            parser.oninstruction = jasmine.createSpy('oninstruction');
+            parser.receive('5.te' + LOW_SURROGATE + 'st,'
+                + '5.12' + HIGH_SURROGATE + '3' + LOW_SURROGATE + ','
+                + '6.5' + LOW_SURROGATE + LOW_SURROGATE + '4' + HIGH_SURROGATE + HIGH_SURROGATE + ','
+                + '10.' + UTF8_MULTIBYTE + HIGH_SURROGATE + UTF8_MULTIBYTE + HIGH_SURROGATE + ';',
+            );
+            expect(parser.oninstruction).toHaveBeenCalledOnceWith('te' + LOW_SURROGATE + 'st', [
+                '12' + HIGH_SURROGATE + '3' + LOW_SURROGATE,
+                '5' + LOW_SURROGATE + LOW_SURROGATE + '4' + HIGH_SURROGATE + HIGH_SURROGATE,
+                UTF8_MULTIBYTE + HIGH_SURROGATE + UTF8_MULTIBYTE + HIGH_SURROGATE
+            ]);
+        });
+
+    });
+
+    // Instruction fed via blocks of characters that accumulate via an external
+    // buffer
+    describe('when an instruction is received via an external buffer', function() {
+
+        it('should correctly parse each element and invoke oninstruction once ready', function() {
+            parser.oninstruction = jasmine.createSpy('oninstruction');
+            parser.receive('5.test2,10.hello', true);
+            expect(parser.oninstruction).not.toHaveBeenCalled();
+            parser.receive('5.test2,10.hellohello,15', true);
+            expect(parser.oninstruction).not.toHaveBeenCalled();
+            parser.receive('5.test2,10.hellohello,15.worldworldworld;', true);
+            expect(parser.oninstruction).toHaveBeenCalledOnceWith('test2', [ 'hellohello', 'worldworldworld' ]);
+        });
+
+    });
+
+    // Verify codePointCount() utility function correctly counts codepoints in
+    // full strings
+    describe('when a string is provided to codePointCount()', function() {
+
+        it('should return the number of codepoints in that string', function() {
+            expect(Guacamole.Parser.codePointCount('')).toBe(0);
+            expect(Guacamole.Parser.codePointCount('test string')).toBe(11);
+            expect(Guacamole.Parser.codePointCount('surrogate' + SURROGATE_PAIR + 'pair')).toBe(14);
+            expect(Guacamole.Parser.codePointCount('missing' + HIGH_SURROGATE + 'surrogates' + LOW_SURROGATE)).toBe(19);
+            expect(Guacamole.Parser.codePointCount(HIGH_SURROGATE + LOW_SURROGATE + HIGH_SURROGATE)).toBe(2);
+            expect(Guacamole.Parser.codePointCount(HIGH_SURROGATE + HIGH_SURROGATE + LOW_SURROGATE)).toBe(2);
+        });
+
+    });
+
+    // Verify codePointCount() utility function correctly counts codepoints in
+    // substrings
+    describe('when a substring is provided to codePointCount()', function() {
+
+        it('should return the number of codepoints in that substring', function() {
+            expect(Guacamole.Parser.codePointCount('test string', 0)).toBe(11);
+            expect(Guacamole.Parser.codePointCount('surrogate' + SURROGATE_PAIR + 'pair', 5)).toBe(9);
+            expect(Guacamole.Parser.codePointCount('missing' + HIGH_SURROGATE + 'surrogates' + LOW_SURROGATE, 2, 17)).toBe(15);
+            expect(Guacamole.Parser.codePointCount(HIGH_SURROGATE + LOW_SURROGATE + HIGH_SURROGATE, 0, 2)).toBe(1);
+            expect(Guacamole.Parser.codePointCount(HIGH_SURROGATE + HIGH_SURROGATE + LOW_SURROGATE, 1, 2)).toBe(1);
+        });
+
+    });
+
+    // Verify toInstruction() utility function correctly encodes instructions
+    describe('when an array of elements is provided to toInstruction()', function() {
+
+        it('should return a correctly-encoded Guacamole instruction', function() {
+            expect(Guacamole.Parser.toInstruction([ 'test', 'instruction' ])).toBe('4.test,11.instruction;');
+            expect(Guacamole.Parser.toInstruction([ 'test' + SURROGATE_PAIR, 'instruction' ]))
+                    .toBe('5.test' + SURROGATE_PAIR + ',11.instruction;');
+            expect(Guacamole.Parser.toInstruction([ UTF8_MULTIBYTE, HIGH_SURROGATE + 'xyz' + LOW_SURROGATE ]))
+                    .toBe('4.' + UTF8_MULTIBYTE + ',5.' + HIGH_SURROGATE + 'xyz' + LOW_SURROGATE + ';');
+            expect(Guacamole.Parser.toInstruction([ UTF8_MULTIBYTE, LOW_SURROGATE + 'xyz' + HIGH_SURROGATE ]))
+                    .toBe('4.' + UTF8_MULTIBYTE + ',5.' + LOW_SURROGATE + 'xyz' + HIGH_SURROGATE + ';');
+        });
+
+    });
+
+});

guacamole-common/src/main/java/org/apache/guacamole/protocol/GuacamoleInstruction.java

@@ -93,6 +93,22 @@ public class GuacamoleInstruction {
         return args;
     }
 
+    /**
+     * Appends the given value to the provided StringBuilder as a Guacamole
+     * instruction element, including length prefix.
+     *
+     * @param buff
+     *     The StringBuilder to append the element to.
+     *
+     * @param element
+     *     The string value of the element to append.
+     */
+    private static void appendElement(StringBuilder buff, String element) {
+        buff.append(element.codePointCount(0, element.length()));
+        buff.append('.');
+        buff.append(element);
+    }
+
     /**
      * Returns this GuacamoleInstruction in the form it would be sent over the
      * Guacamole protocol.
@@ -111,16 +127,12 @@ public class GuacamoleInstruction {
             StringBuilder buff = new StringBuilder();
 
             // Write opcode
-            buff.append(opcode.length());
+            appendElement(buff, opcode);
-            buff.append('.');
-            buff.append(opcode);
 
             // Write argument values
             for (String value : args) {
                 buff.append(',');
-                buff.append(value.length());
+                appendElement(buff, value);
-                buff.append('.');
-                buff.append(value);
             }
 
             // Write terminator

guacamole-common/src/main/java/org/apache/guacamole/protocol/GuacamoleParser.java

@@ -21,7 +21,6 @@ package org.apache.guacamole.protocol;
 
 import java.util.Arrays;
 import java.util.Iterator;
-import java.util.List;
 import org.apache.guacamole.GuacamoleException;
 import org.apache.guacamole.GuacamoleServerException;
 
@@ -87,10 +86,18 @@ public class GuacamoleParser implements Iterator<GuacamoleInstruction> {
     private State state = State.PARSING_LENGTH;
 
     /**
-     * The length of the current element, if known.
+     * The length of the current element, if known, in Java characters. This
+     * value may be adjusted as an element is parsed to take surrogates into
+     * account.
      */
     private int elementLength = 0;
 
+    /**
+     * The length of the current element, if known, in Unicode codepoints. This
+     * value will NOT change as an element is parsed.
+     */
+    private int elementCodepoints;
+
     /**
      * The number of elements currently parsed.
      */
@@ -104,13 +111,22 @@ public class GuacamoleParser implements Iterator<GuacamoleInstruction> {
     /**
      * Appends data from the given buffer to the current instruction.
      * 
-     * @param chunk The buffer containing the data to append.
+     * @param chunk
-     * @param offset The offset within the buffer where the data begins.
+     *     The buffer containing the data to append.
-     * @param length The length of the data to append.
+     *
-     * @return The number of characters appended, or 0 if complete instructions
+     * @param offset
-     *         have already been parsed and must be read via next() before
+     *     The offset within the buffer where the data begins.
-     *         more data can be appended.
+     *
-     * @throws GuacamoleException If an error occurs while parsing the new data.
+     * @param length
+     *     The length of the data to append.
+     *
+     * @return
+     *     The number of characters appended, or 0 if complete instructions
+     *     have already been parsed and must be read via next() before more
+     *     data can be appended.
+     *
+     * @throws GuacamoleException
+     *     If an error occurs while parsing the new data.
      */
     public int append(char chunk[], int offset, int length) throws GuacamoleException {
 
@@ -156,39 +172,63 @@ public class GuacamoleParser implements Iterator<GuacamoleInstruction> {
             }
 
             // Save length
-            elementLength = parsedLength;
+            elementCodepoints = elementLength = parsedLength;
 
         } // end parse length
 
         // Parse element content, if available
-        if (state == State.PARSING_CONTENT && charsParsed + elementLength + 1 <= length) {
+        while (state == State.PARSING_CONTENT && charsParsed + elementLength + 1 <= length) {
 
-            // Read element
+            // Read element (which may not match element length if surrogate
+            // characters are present)
             String element = new String(chunk, offset + charsParsed, elementLength);
+
+            // Verify element contains the number of whole Unicode characters
+            // expected, scheduling a future read if we don't yet have enough
+            // characters
+            int codepoints = element.codePointCount(0, element.length());
+            if (codepoints < elementCodepoints) {
+                elementLength += elementCodepoints - codepoints;
+                continue;
+            }
+
+            // If the current element ends with a character involving both
+            // a high and low surrogate, elementLength points to the low
+            // surrogate and NOT the element terminator. We must correct the
+            // length and reevaluate.
+            else if (Character.isSurrogatePair(chunk[offset + charsParsed + elementLength - 1],
+                    chunk[offset + charsParsed + elementLength])) {
+                elementLength++;
+                continue;
+            }
+
             charsParsed += elementLength;
             elementLength = 0;
 
-            // Read terminator char following element
-            char terminator = chunk[offset + charsParsed++];
-
             // Add element to currently parsed elements
             elements[elementCount++] = element;
-            
-            // If semicolon, store end-of-instruction
-            if (terminator == ';') {
-                state = State.COMPLETE;
-                parsedInstruction = new GuacamoleInstruction(elements[0],
-                        Arrays.asList(elements).subList(1, elementCount));
-            }
 
-            // If comma, move on to next element
+            // Read terminator char following element
-            else if (terminator == ',')
+            char terminator = chunk[offset + charsParsed++];
-                state = State.PARSING_LENGTH;
+            switch (terminator) {
+
+                // If semicolon, store end-of-instruction
+                case ';':
+                    state = State.COMPLETE;
+                    parsedInstruction = new GuacamoleInstruction(elements[0],
+                            Arrays.asList(elements).subList(1, elementCount));
+                    break;
+
+                // If comma, move on to next element
+                case ',':
+                    state = State.PARSING_LENGTH;
+                    break;
+
+                // Otherwise, parse error
+                default:
+                    state = State.ERROR;
+                    throw new GuacamoleServerException("Element terminator of instruction was not ';' nor ','");
 
-            // Otherwise, parse error
-            else {
-                state = State.ERROR;
-                throw new GuacamoleServerException("Element terminator of instruction was not ';' nor ','");
             }
 
         } // end parse content

guacamole-common/src/test/java/org/apache/guacamole/protocol/GuacamoleInstructionTest.java

@@ -0,0 +1,205 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.protocol;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import static org.junit.Assert.*;
+import org.junit.Test;
+
+/**
+ * Unit test for GuacamoleParser. Verifies that parsing of the Guacamole
+ * protocol works as required.
+ */
+public class GuacamoleInstructionTest {
+
+    /**
+     * A single test case for verifying that Guacamole protocol implementations
+     * correctly parse or encode Guacamole instructions.
+     */
+    public static class TestCase extends GuacamoleInstruction {
+
+        /**
+         * The full and correct Guacamole protocol representation of this
+         * instruction.
+         */
+        public final String UNPARSED;
+
+        /**
+         * The opcode that should be present in the Guacamole instruction;
+         */
+        public final String OPCODE;
+
+        /**
+         * All arguments that should be present in the Guacamole instruction;
+         */
+        public final List<String> ARGS;
+
+        /**
+         * Creates a new TestCase representing the given Guacamole instruction.
+         *
+         * @param unparsed
+         *     The full and correct Guacamole protocol representation of this
+         *     instruction.
+         *
+         * @param opcode
+         *     The opcode of the Guacamole instruction.
+         *
+         * @param args
+         *     The arguments of the Guacamole instruction, if any.
+         */
+        public TestCase(String unparsed, String opcode, String... args) {
+            super(opcode, Arrays.copyOf(args, args.length));
+            this.UNPARSED = unparsed;
+            this.OPCODE = opcode;
+            this.ARGS = Collections.unmodifiableList(Arrays.asList(args));
+        }
+
+    }
+
+    /**
+     * A single Unicode high surrogate character (any character between U+D800
+     * and U+DB7F).
+     */
+    public static final String HIGH_SURROGATE = "\uD802";
+
+    /**
+     * A single Unicode low surrogate character (any character between U+DC00
+     * and U+DFFF).
+     */
+    public static final String LOW_SURROGATE = "\uDF00";
+
+    /**
+     * A Unicode surrogate pair, consisting of a high and low surrogate.
+     */
+    public static final String SURROGATE_PAIR = HIGH_SURROGATE + LOW_SURROGATE;
+
+    /**
+     * A 4-character test string containing Unicode characters that require
+     * multiple bytes when encoded as UTF-8, including at least one character
+     * that is encoded as a surrogate pair in UTF-16.
+     */
+    public static final String UTF8_MULTIBYTE = "\u72AC" + SURROGATE_PAIR + "z\u00C1";
+
+    /**
+     * Pre-defined set of test cases for verifying Guacamole instructions are
+     * correctly parsed and encoded.
+     */
+    public static List<TestCase> TEST_CASES = Collections.unmodifiableList(Arrays.asList(
+
+        // Empty instruction
+        new TestCase(
+            "0.;",
+            ""
+        ),
+
+        // Instruction using basic Latin characters
+        new TestCase(
+
+              "5.test2,"
+            + "10.hellohello,"
+            + "15.worldworldworld;",
+
+            "test2",
+            "hellohello",
+            "worldworldworld"
+
+        ),
+
+        // Instruction using characters requiring multiple bytes in UTF-8 and
+        // surrogate pairs in UTF-16, including an element ending with a surrogate
+        // pair
+        new TestCase(
+
+              "4.ab" + HIGH_SURROGATE + HIGH_SURROGATE + ","
+            + "6.a" + UTF8_MULTIBYTE + "b,"
+            + "5.12345,"
+            + "10.a" + UTF8_MULTIBYTE + UTF8_MULTIBYTE + "c;",
+
+            "ab" + HIGH_SURROGATE + HIGH_SURROGATE,
+            "a" + UTF8_MULTIBYTE + "b",
+            "12345",
+            "a" + UTF8_MULTIBYTE + UTF8_MULTIBYTE + "c"
+
+        ),
+
+        // Instruction with an element values ending with an incomplete surrogate
+        // pair (high or low surrogate only)
+        new TestCase(
+
+              "4.test,"
+            + "5.1234" + HIGH_SURROGATE + ","
+            + "5.4567" + LOW_SURROGATE + ";",
+
+            "test",
+            "1234" + HIGH_SURROGATE,
+            "4567" + LOW_SURROGATE
+
+        ),
+
+        // Instruction with element values containing incomplete surrogate pairs
+        new TestCase(
+
+              "5.te" + LOW_SURROGATE + "st,"
+            + "5.12" + HIGH_SURROGATE + "3" + LOW_SURROGATE + ","
+            + "6.5" + LOW_SURROGATE + LOW_SURROGATE + "4" + HIGH_SURROGATE + HIGH_SURROGATE + ","
+            + "10." + UTF8_MULTIBYTE + HIGH_SURROGATE + UTF8_MULTIBYTE + HIGH_SURROGATE + ";",
+
+            "te" + LOW_SURROGATE + "st",
+            "12" + HIGH_SURROGATE + "3" + LOW_SURROGATE,
+            "5" + LOW_SURROGATE + LOW_SURROGATE + "4" + HIGH_SURROGATE + HIGH_SURROGATE,
+            UTF8_MULTIBYTE + HIGH_SURROGATE + UTF8_MULTIBYTE + HIGH_SURROGATE
+
+        )
+
+    ));
+
+    /**
+     * Verifies that instruction opcodes are represented correctly.
+     */
+    @Test
+    public void testGetOpcode() {
+        for (TestCase testCase : TEST_CASES) {
+            assertEquals(testCase.OPCODE, testCase.getOpcode());
+        }
+    }
+
+    /**
+     * Verifies that instruction arguments are represented correctly.
+     */
+    @Test
+    public void testGetArgs() {
+        for (TestCase testCase : TEST_CASES) {
+            assertEquals(testCase.ARGS, testCase.getArgs());
+        }
+    }
+
+    /**
+     * Verifies that instructions are encoded correctly.
+     */
+    @Test
+    public void testToString() {
+        for (TestCase testCase : TEST_CASES) {
+            assertEquals(testCase.UNPARSED, testCase.toString());
+        }
+    }
+
+}

guacamole-common/src/test/java/org/apache/guacamole/protocol/GuacamoleParserTest.java

@@ -20,6 +20,8 @@
 package org.apache.guacamole.protocol;
 
 import org.apache.guacamole.GuacamoleException;
+import static org.apache.guacamole.protocol.GuacamoleInstructionTest.TEST_CASES;
+import org.apache.guacamole.protocol.GuacamoleInstructionTest.TestCase;
 import static org.junit.Assert.*;
 import org.junit.Test;
 
@@ -35,82 +37,46 @@ public class GuacamoleParserTest {
     private final GuacamoleParser parser = new GuacamoleParser();
 
     /**
-     * Test of append method, of class GuacamoleParser.
+     * Verify that GuacamoleParser correctly parses each of the instruction
-     * 
+     * test cases included in the GuacamoleInstruction test.
-     * @throws GuacamoleException If a parse error occurs while parsing the
+     *
-     *                            known-good test string.
+     * @throws GuacamoleException
+     *     If a parse error occurs.
      */
     @Test
     public void testParser() throws GuacamoleException {
 
-        // Test string
+        // Build buffer containing all of the instruction test cases, one after
-        char buffer[] = "1.a,2.bc,3.def,10.helloworld;4.test,5.test2;0.;3.foo;".toCharArray();
+        // the other
+        StringBuilder allTestCases = new StringBuilder();
+        for (TestCase testCase : TEST_CASES)
+            allTestCases.append(testCase.UNPARSED);
+
+        // Prepare buffer and offsets for feeding the data into the parser as
+        // if received over the network
+        char buffer[] = allTestCases.toString().toCharArray();
         int offset = 0;
         int length = buffer.length;
 
-        GuacamoleInstruction instruction;
+        // Verify that each of the expected instructions is received in order
-        int parsed;
+        for (TestCase testCase : TEST_CASES) {
 
-        // Parse more data
+            // Feed data into parser until parser refuses to receive more data
-        while (length > 0 && (parsed = parser.append(buffer, offset, length)) != 0) {
+            int parsed;
-            offset += parsed;
+            while (length > 0 && (parsed = parser.append(buffer, offset, length)) != 0) {
-            length -= parsed;
+                offset += parsed;
-        }
+                length -= parsed;
+            }
 
-        // Validate first test instruction
+            // An instruction should now be parsed and ready for retrieval
-        assertTrue(parser.hasNext());
+            assertTrue(parser.hasNext());
-        instruction = parser.next();
-        assertNotNull(instruction);
-        assertEquals(3, instruction.getArgs().size());
-        assertEquals("a", instruction.getOpcode());
-        assertEquals("bc", instruction.getArgs().get(0));
-        assertEquals("def", instruction.getArgs().get(1));
-        assertEquals("helloworld", instruction.getArgs().get(2));
 
-        // Parse more data
+            // Verify instruction contains expected opcode and args
-        while (length > 0 && (parsed = parser.append(buffer, offset, length)) != 0) {
+            GuacamoleInstruction instruction = parser.next();
-            offset += parsed;
+            assertNotNull(instruction);
-            length -= parsed;
+            assertEquals(testCase.OPCODE, instruction.getOpcode());
-        }
+            assertEquals(testCase.ARGS, instruction.getArgs());
 
-        // Validate second test instruction
-        assertTrue(parser.hasNext());
-        instruction = parser.next();
-        assertNotNull(instruction);
-        assertEquals(1, instruction.getArgs().size());
-        assertEquals("test", instruction.getOpcode());
-        assertEquals("test2", instruction.getArgs().get(0));
-
-        // Parse more data
-        while (length > 0 && (parsed = parser.append(buffer, offset, length)) != 0) {
-            offset += parsed;
-            length -= parsed;
-        }
-
-        // Validate third test instruction
-        assertTrue(parser.hasNext());
-        instruction = parser.next();
-        assertNotNull(instruction);
-        assertEquals(0, instruction.getArgs().size());
-        assertEquals("", instruction.getOpcode());
-
-        // Parse more data
-        while (length > 0 && (parsed = parser.append(buffer, offset, length)) != 0) {
-            offset += parsed;
-            length -= parsed;
-        }
-
-        // Validate fourth test instruction
-        assertTrue(parser.hasNext());
-        instruction = parser.next();
-        assertNotNull(instruction);
-        assertEquals(0, instruction.getArgs().size());
-        assertEquals("foo", instruction.getOpcode());
-
-        // Parse more data
-        while (length > 0 && (parsed = parser.append(buffer, offset, length)) != 0) {
-            offset += parsed;
-            length -= parsed;
         }
 
         // There should be no more instructions