From ef067b26242baf07ee8f9f3dbde85329db0c20a0 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 27 Aug 2015 11:12:51 -0700 Subject: [PATCH] 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 --- .../jetty/websocket/api/CloseStatus.java | 25 +++-- .../jetty/websocket/common/CloseInfo.java | 94 +++++++++++-------- .../websocket/common/WebSocketSession.java | 2 +- .../jetty/websocket/common/io/IOState.java | 3 - 4 files changed, 70 insertions(+), 54 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 417221e9261..e6a3e895aac 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; } + + byte[] reasonBytes = reason.getBytes(StandardCharsets.UTF_8); + if (reasonBytes.length > MAX_REASON_PHRASE) + { + byte[] trimmed = new byte[MAX_REASON_PHRASE]; + System.arraycopy(reasonBytes,0,trimmed,0,MAX_REASON_PHRASE); + return new String(trimmed,StandardCharsets.UTF_8); + } - if (reason.length() > MAX_REASON_PHRASE) - { - return reason.substring(0,MAX_REASON_PHRASE); - } - else - { - return reason; - } + 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 bf8d236e14d..ef064e7708f 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,34 +80,24 @@ public class CloseInfo if (data.remaining() > 0) { - // Reason - try + // 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) { - Utf8StringBuilder utf = new Utf8StringBuilder(); - utf.append(data); - reason = utf.toString(); - } - catch (NotUtf8Exception e) - { - if (validate) + try + { + Utf8StringBuilder utf = new Utf8StringBuilder(); + // if this throws, we know we have bad UTF8 + utf.append(reasonBytes,0,reasonBytes.length); + } + catch (NotUtf8Exception e) { throw new BadPayloadException("Invalid Close Reason",e); } - else - { - LOG.warn(e); - } - } - catch (RuntimeException e) - { - if (validate) - { - throw new ProtocolException("Invalid Close Reason",e); - } - else - { - LOG.warn(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 568b889f0bb..90645265950 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 6a8851d4678..5ba0eb9f164 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);