476023 - Incorrect trimming of WebSocket close reason

+ Fixed CloseStatus.trimMaxReasonLength() to perform trim correctly
+ Deprecated CloseStatus.trimMaxReasonLength() because it creates
  to many objects
+ Removed use of CloseStatus.trimMaxReasonLength() in implementation
  code
+ Improved CloseInfo ...
  + tracks reason via utf8 byte array (not String object)
  + trims utf8 byte array as-needed
+ All non-jsr implementations use CloseInfo logic

4 files changed, 68 insertions, 52 deletions

diff --git a/jetty-websocket/websocket-api/src/main/java/org/eclipse/jetty/websocket/api/CloseStatus.java b/jetty-websocket/websocket-api/src/main/java/org/eclipse/jetty/websocket/api/CloseStatus.java
index 417221e926..e6a3e895aa 100644
--- a/jetty-websocket/websocket-api/src/main/java/org/eclipse/jetty/websocket/api/CloseStatus.java
+++ b/jetty-websocket/websocket-api/src/main/java/org/eclipse/jetty/websocket/api/CloseStatus.java
@@ -18,33 +18,38 @@
 
 package org.eclipse.jetty.websocket.api;
 
+import java.nio.charset.StandardCharsets;
+
 public class CloseStatus
 {
     private static final int MAX_CONTROL_PAYLOAD = 125;
-    private static final int MAX_REASON_PHRASE = MAX_CONTROL_PAYLOAD - 2;
+    public static final int MAX_REASON_PHRASE = MAX_CONTROL_PAYLOAD - 2;
 
     /**
-     * Convenience method for trimming a long reason phrase at the maximum reason phrase length.
+     * Convenience method for trimming a long reason phrase at the maximum reason phrase length of 123 UTF-8 bytes (per WebSocket spec).
      * 
      * @param reason
      *            the proposed reason phrase
      * @return the reason phrase (trimmed if needed)
+     * @deprecated use of this method is strongly discouraged, as it creates too many new objects that are just thrown away to accomplish its goals.
      */
+    @Deprecated
     public static String trimMaxReasonLength(String reason)
     {
         if (reason == null)
         {
             return null;
         }
-        
-        if (reason.length() > MAX_REASON_PHRASE)
-        {
-            return reason.substring(0,MAX_REASON_PHRASE);
-        }
-        else
+
+        byte[] reasonBytes = reason.getBytes(StandardCharsets.UTF_8);
+        if (reasonBytes.length > MAX_REASON_PHRASE)
         {
-            return reason;
+            byte[] trimmed = new byte[MAX_REASON_PHRASE];
+            System.arraycopy(reasonBytes,0,trimmed,0,MAX_REASON_PHRASE);
+            return new String(trimmed,StandardCharsets.UTF_8);
         }
+        
+        return reason;
     }
 
     private int code;
diff --git a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/CloseInfo.java b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/CloseInfo.java
index bf8d236e14..ef064e7708 100644
--- a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/CloseInfo.java
+++ b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/CloseInfo.java
@@ -19,14 +19,13 @@
 package org.eclipse.jetty.websocket.common;
 
 import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
 
 import org.eclipse.jetty.util.BufferUtil;
-import org.eclipse.jetty.util.StringUtil;
-import org.eclipse.jetty.util.Utf8Appendable.NotUtf8Exception;
 import org.eclipse.jetty.util.Utf8StringBuilder;
-import org.eclipse.jetty.util.log.Log;
-import org.eclipse.jetty.util.log.Logger;
+import org.eclipse.jetty.util.Utf8Appendable.NotUtf8Exception;
 import org.eclipse.jetty.websocket.api.BadPayloadException;
+import org.eclipse.jetty.websocket.api.CloseStatus;
 import org.eclipse.jetty.websocket.api.ProtocolException;
 import org.eclipse.jetty.websocket.api.StatusCode;
 import org.eclipse.jetty.websocket.api.extensions.Frame;
@@ -34,19 +33,23 @@ import org.eclipse.jetty.websocket.common.frames.CloseFrame;
 
 public class CloseInfo
 {
-    private static final Logger LOG = Log.getLogger(CloseInfo.class);
     private int statusCode;
-    private String reason;
+    private byte[] reasonBytes;
 
     public CloseInfo()
     {
         this(StatusCode.NO_CODE,null);
     }
 
+    /**
+     * Parse the Close Frame payload.
+     * 
+     * @param payload the raw close frame payload.
+     * @param validate true if payload should be validated per WebSocket spec.
+     */
     public CloseInfo(ByteBuffer payload, boolean validate)
     {
         this.statusCode = StatusCode.NO_CODE;
-        this.reason = null;
 
         if ((payload == null) || (payload.remaining() == 0))
         {
@@ -77,33 +80,23 @@ public class CloseInfo
 
             if (data.remaining() > 0)
             {
-                // Reason
-                try
-                {
-                    Utf8StringBuilder utf = new Utf8StringBuilder();
-                    utf.append(data);
-                    reason = utf.toString();
-                }
-                catch (NotUtf8Exception e)
-                {
-                    if (validate)
-                    {
-                        throw new BadPayloadException("Invalid Close Reason",e);
-                    }
-                    else
-                    {
-                        LOG.warn(e);
-                    }
-                }
-                catch (RuntimeException e)
+                // Reason (trimmed to max reason size)
+                int len = Math.min(data.remaining(), CloseStatus.MAX_REASON_PHRASE);
+                reasonBytes = new byte[len];
+                data.get(reasonBytes,0,len);
+                
+                // Spec Requirement : throw BadPayloadException on invalid UTF8
+                if(validate)
                 {
-                    if (validate)
+                    try
                     {
-                        throw new ProtocolException("Invalid Close Reason",e);
+                        Utf8StringBuilder utf = new Utf8StringBuilder();
+                        // if this throws, we know we have bad UTF8
+                        utf.append(reasonBytes,0,reasonBytes.length);
                     }
-                    else
+                    catch (NotUtf8Exception e)
                     {
-                        LOG.warn(e);
+                        throw new BadPayloadException("Invalid Close Reason",e);
                     }
                 }
             }
@@ -125,10 +118,28 @@ public class CloseInfo
         this(statusCode,null);
     }
 
+    /**
+     * Create a CloseInfo, trimming the reason to {@link CloseStatus#MAX_REASON_PHRASE} UTF-8 bytes if needed.
+     * 
+     * @param statusCode the status code
+     * @param reason the raw reason code
+     */
     public CloseInfo(int statusCode, String reason)
     {
         this.statusCode = statusCode;
-        this.reason = reason;
+        if (reason != null)
+        {
+            byte[] utf8Bytes = reason.getBytes(StandardCharsets.UTF_8);
+            if (utf8Bytes.length > CloseStatus.MAX_REASON_PHRASE)
+            {
+                this.reasonBytes = new byte[CloseStatus.MAX_REASON_PHRASE];
+                System.arraycopy(utf8Bytes,0,this.reasonBytes,0,CloseStatus.MAX_REASON_PHRASE);
+            }
+            else
+            {
+                this.reasonBytes = utf8Bytes;
+            }
+        }
     }
 
     private ByteBuffer asByteBuffer()
@@ -140,11 +151,10 @@ public class CloseInfo
         }
 
         int len = 2; // status code
-        byte utf[] = null;
-        if (StringUtil.isNotBlank(reason))
+        boolean hasReason = (this.reasonBytes != null) && (this.reasonBytes.length > 0);
+        if (hasReason)
         {
-            utf = StringUtil.getUtf8Bytes(reason);
-            len += utf.length;
+            len += this.reasonBytes.length;
         }
 
         ByteBuffer buf = BufferUtil.allocate(len);
@@ -152,9 +162,9 @@ public class CloseInfo
         buf.put((byte)((statusCode >>> 8) & 0xFF));
         buf.put((byte)((statusCode >>> 0) & 0xFF));
 
-        if (utf != null)
+        if (hasReason)
         {
-            buf.put(utf,0,utf.length);
+            buf.put(this.reasonBytes,0,this.reasonBytes.length);
         }
         BufferUtil.flipToFlush(buf,0);
 
@@ -178,7 +188,11 @@ public class CloseInfo
 
     public String getReason()
     {
-        return reason;
+        if (this.reasonBytes == null)
+        {
+            return null;
+        }
+        return new String(this.reasonBytes,StandardCharsets.UTF_8);
     }
 
     public int getStatusCode()
@@ -199,6 +213,6 @@ public class CloseInfo
     @Override
     public String toString()
     {
-        return String.format("CloseInfo[code=%d,reason=%s]",statusCode,reason);
+        return String.format("CloseInfo[code=%d,reason=%s]",statusCode,getReason());
     }
 }
diff --git a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java
index 568b889f0b..9064526595 100644
--- a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java
+++ b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/WebSocketSession.java
@@ -105,7 +105,7 @@ public class WebSocketSession extends ContainerLifeCycle implements Session, Inc
     @Override
     public void close(int statusCode, String reason)
     {
-        connection.close(statusCode,CloseStatus.trimMaxReasonLength(reason));
+        connection.close(statusCode,reason);
     }
 
     /**
diff --git a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/IOState.java b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/IOState.java
index 6a8851d467..5ba0eb9f16 100644
--- a/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/IOState.java
+++ b/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/io/IOState.java
@@ -27,7 +27,6 @@ import java.util.concurrent.atomic.AtomicReference;
 import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
-import org.eclipse.jetty.websocket.api.CloseStatus;
 import org.eclipse.jetty.websocket.api.StatusCode;
 import org.eclipse.jetty.websocket.common.CloseInfo;
 import org.eclipse.jetty.websocket.common.ConnectionState;
@@ -458,7 +457,6 @@ public class IOState
                 }
             }
 
-            reason = CloseStatus.trimMaxReasonLength(reason);
             CloseInfo close = new CloseInfo(StatusCode.ABNORMAL,reason);
 
             finalClose.compareAndSet(null,close);
@@ -509,7 +507,6 @@ public class IOState
                 }
             }
 
-            reason = CloseStatus.trimMaxReasonLength(reason);
             CloseInfo close = new CloseInfo(StatusCode.ABNORMAL,reason);
 
             finalClose.compareAndSet(null,close);