Enhance header name validation exception message (#2924)

Motivation: Provide more details about a illegal header name. Modifications: - Use `TokenValidatingProcessor` that captures more context for generating an exception message, if necessary; - Add benchmark to measure performance before/after change; Result: Users can see what header name has an illegal character as well as the position (index) of an illegal character.
apple · May 21, 2024 · 2f50382 · 2f50382
1 parent 54ec680
commit 2f50382
Show file tree

Hide file tree

Showing 7 changed files with 193 additions and 43 deletions.
diff --git a/...marks/src/jmh/java/io/servicetalk/http/api/DefaultHttpHeadersNameValidationBenchmark.java b/...marks/src/jmh/java/io/servicetalk/http/api/DefaultHttpHeadersNameValidationBenchmark.java
@@ -0,0 +1,86 @@
+/*
+ * Copyright © 2024 Apple Inc. and the ServiceTalk project authors
+ *
+ * Licensed 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 io.servicetalk.http.api;
+
+import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.BenchmarkMode;
+import org.openjdk.jmh.annotations.Fork;
+import org.openjdk.jmh.annotations.Level;
+import org.openjdk.jmh.annotations.Measurement;
+import org.openjdk.jmh.annotations.Mode;
+import org.openjdk.jmh.annotations.Param;
+import org.openjdk.jmh.annotations.Scope;
+import org.openjdk.jmh.annotations.Setup;
+import org.openjdk.jmh.annotations.State;
+import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
+
+import java.util.Arrays;
+
+/*
+ * This benchmark measures performance of header name validation in DefaultHttpHeaders:
+ *
+ * ByteProcessor as a method-reference:
+ * Benchmark                                            (count)  (invalid)   Mode  Cnt        Score        Error  Units
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader        4      false  thrpt    5  8438734.025 ± 165801.745  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader        4       true  thrpt    5   339720.292 ±   5379.742  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader        8      false  thrpt    5  4833763.689 ±  31037.631  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader        8       true  thrpt    5   167820.982 ±    349.719  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader       16      false  thrpt    5  2537292.742 ±  11400.611  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader       16       true  thrpt    5    84582.812 ±    168.087  ops/s
+ *
+ * ByteProcessor as an instance of a static inner class with extra state:
+ * Benchmark                                            (count)  (invalid)   Mode  Cnt        Score        Error  Units
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader        4      false  thrpt    5  8562586.842 ± 210534.848  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader        4       true  thrpt    5   274093.669 ±   1818.531  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader        8      false  thrpt    5  5024017.665 ±  32830.519  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader        8       true  thrpt    5   135011.887 ±   1832.187  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader       16      false  thrpt    5  2523554.519 ±   8780.660  ops/s
+ * DefaultHttpHeadersNameValidationBenchmark.addHeader       16       true  thrpt    5    68474.840 ±   1154.751  ops/s
+ */
+@Fork(1)
+@State(Scope.Benchmark)
+@Warmup(iterations = 5, time = 5)
+@Measurement(iterations = 5, time = 10)
+@BenchmarkMode(Mode.Throughput)
+public class DefaultHttpHeadersNameValidationBenchmark {
+
+    @Param({"4", "8", "16"})
+    private int count;
+    @Param({"false", "true"})
+    private boolean invalid;
+    private String[] names;
+
+    @Setup(Level.Trial)
+    public void setup() {
+        String baseValue = "header-" + (invalid ? (char) 0 : 'x') + "-name-";
+        names = new String[count];
+        Arrays.setAll(names, i -> baseValue + i);
+    }
+
+    @Benchmark
+    public HttpHeaders addHeader(Blackhole blackhole) {
+        HttpHeaders headers = DefaultHttpHeadersFactory.INSTANCE.newHeaders();
+        for (String name : names) {
+            try {
+                headers.add(name, "value");
+            } catch (IllegalArgumentException e) {
+                blackhole.consume(e);
+            }
+        }
+        return headers;
+    }
+}
diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpHeaders.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpHeaders.java
@@ -588,7 +588,7 @@ protected CharSequence validateValue(final CharSequence value) {
      * @param name The filed-name to validate.
      */
     private static void validateHeaderName(final CharSequence name) {
-        validateToken(name);
+        validateToken(name, "header name");
     }
 
     /**

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpSetCookie.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/DefaultHttpSetCookie.java
@@ -153,7 +153,7 @@ static HttpSetCookie parseSetCookie(final CharSequence setCookieString, boolean
                         }
                         name = setCookieString.subSequence(begin, i);
                         if (validateContent) {
-                            validateToken(name);
+                            validateToken(name, "set-cookie name");
                         }
                         parseState = ParseState.ParsingValue;
                     } else if (parseState == ParseState.Unknown) {

diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HeaderUtils.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HeaderUtils.java
@@ -15,6 +15,7 @@
  */
 package io.servicetalk.http.api;
 
+import io.servicetalk.buffer.api.ByteProcessor;
 import io.servicetalk.encoding.api.ContentCodec;
 import io.servicetalk.encoding.api.Identity;
 import io.servicetalk.serialization.api.SerializationException;
@@ -52,6 +53,7 @@
 import static io.servicetalk.utils.internal.NetworkUtils.isValidIpV4Address;
 import static io.servicetalk.utils.internal.NetworkUtils.isValidIpV6Address;
 import static java.lang.Boolean.parseBoolean;
+import static java.lang.Integer.toHexString;
 import static java.lang.Math.min;
 import static java.lang.System.getProperty;
 import static java.lang.System.lineSeparator;
@@ -238,9 +240,10 @@ static void validateCookieNameAndValue(final CharSequence cookieName, final Char
      * <a href="https://tools.ietf.org/html/rfc7231#section-4">request method</a>.
      *
      * @param token the token to validate.
+     * @param what name of the field that is validated.
      */
-    static void validateToken(final CharSequence token) {
-        forEachByte(token, HeaderUtils::validateTokenChar);
+    static void validateToken(final CharSequence token, final String what) {
+        forEachByte(token, new TokenValidatingProcessor(token, what));
     }
 
     static void validateHeaderValue(final CharSequence value) {
@@ -796,44 +799,14 @@ private static boolean hasCharset(final CharSequence contentTypeHeader) {
     }
 
     /**
-     * Validate char is a valid <a href="https://tools.ietf.org/html/rfc7230#section-3.2.6">token</a> character.
+     * Validate char is a valid <a href="https://tools.ietf.org/html/rfc7230#section-3.2.6">tchar</a>.
+     * <p>
+     * tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
+     *       / DIGIT / ALPHA
+     *       ; any VCHAR, except delimiters
      *
      * @param value the character to validate.
      */
-    private static boolean validateTokenChar(final byte value) {
-        // HEADER
-        // header-field   = field-name ":" OWS field-value OWS
-        //
-        // field-name     = token
-        // token          = 1*tchar
-        //
-        // tchar          = "!" / "#" / "$" / "%" / "&" / "'" / "*"
-        //                    / "+" / "-" / "." / "^" / "_" / "`" / "|" / "~"
-        //                    / DIGIT / ALPHA
-        //                    ; any VCHAR, except delimiters
-        //  Delimiters are chosen
-        //   from the set of US-ASCII visual characters not allowed in a token
-        //   (DQUOTE and "(),/:;<=>?@[\]{}")
-        //
-        // COOKIE
-        // cookie-pair       = cookie-name "=" cookie-value
-        // cookie-name       = token
-        // token          = 1*<any CHAR except CTLs or separators>
-        // CTL = <any US-ASCII control character
-        //       (octets 0 - 31) and DEL (127)>
-        // separators     = "(" | ")" | "<" | ">" | "@"
-        //                      | "," | ";" | ":" | "\" | <">
-        //                      | "/" | "[" | "]" | "?" | "="
-        //                      | "{" | "}" | SP | HT
-        //
-        // field-name's token is equivalent to cookie-name's token, we can reuse the tchar mask for both:
-        if (!isTchar(value)) {
-            throw new IllegalCharacterException(value,
-                    "! / # / $ / % / & / ' / * / + / - / . / ^ / _ / ` / | / ~ / DIGIT / ALPHA");
-        }
-        return true;
-    }
-
     // visible for testing
     static boolean isTchar(final byte value) {
         return isBitSet(value, TCHAR_LMASK, TCHAR_HMASK);
@@ -864,4 +837,85 @@ private static boolean validateHeaderValue(final byte value) {
         }
         return true;
     }
+
+    private static final class TokenValidatingProcessor implements ByteProcessor {
+
+        private static final String EXPECTED =
+                "! / # / $ / % / & / ' / * / + / - / . / ^ / _ / ` / | / ~ / DIGIT / ALPHA";
+
+        private final CharSequence token;
+        private final String what;
+        private int idx;
+
+        TokenValidatingProcessor(final CharSequence token, final String what) {
+            this.token = token;
+            this.what = what;
+        }
+
+        @Override
+        public boolean process(final byte value) {
+            // HEADER
+            // header-field   = field-name ":" OWS field-value OWS
+            // field-name     = token
+            // token          = 1*tchar
+            //
+            //  Delimiters are chosen
+            //   from the set of US-ASCII visual characters not allowed in a token
+            //   (DQUOTE and "(),/:;<=>?@[\]{}")
+            //
+            // COOKIE
+            // cookie-pair       = cookie-name "=" cookie-value
+            // cookie-name       = token
+            // token          = 1*<any CHAR except CTLs or separators>
+            // CTL = <any US-ASCII control character
+            //       (octets 0 - 31) and DEL (127)>
+            // separators     = "(" | ")" | "<" | ">" | "@"
+            //                      | "," | ";" | ":" | "\" | <">
+            //                      | "/" | "[" | "]" | "?" | "="
+            //                      | "{" | "}" | SP | HT
+            //
+            // field-name's token is equivalent to cookie-name's token, we can reuse the tchar mask for both:
+            if (!isTchar(value)) {
+                throw new IllegalCharacterException(message(value));
+            }
+            ++idx;
+            return true;
+        }
+
+        private String message(final byte value) {
+            final int codePoint = value & 0xff;
+            final int idx = this.idx;
+            final StringBuilder sb = new StringBuilder(40 + intToStringLength(idx) + what.length() +
+                    token.length() + EXPECTED.length())
+                    .append('\'')   // 1
+                    .append((char) codePoint)   // 1
+                    .append("' (0x")    // 5
+                    .append(toHexString(0x100 | codePoint), 1, 3)   // 2 digit hex number
+                    .append(") at index=")  // 11
+                    .append(idx)
+                    .append(" in ") // 4
+                    .append(what)
+                    .append(' ')    // 1
+                    .append('\'')   // 1
+                    .append(token)
+                    .append("', expected [")    // 13
+                    .append(EXPECTED)
+                    .append(']');   // 1
+            return sb.toString();
+        }
+
+        /**
+         * Calculates the length of {@link Integer#toString(int)} for the specified value.
+         */
+        private static int intToStringLength(final int value) {
+            assert value >= 0;
+            int length = 1;
+            int test = 10;
+            while (test <= value && test <= 1_000_000_000) {
+                ++length;
+                test *= 10;
+            }
+            return length;
+        }
+    }
 }
diff --git a/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpRequestMethod.java b/servicetalk-http-api/src/main/java/io/servicetalk/http/api/HttpRequestMethod.java
@@ -95,7 +95,7 @@ private static String validateName(final String name) {
             throw new IllegalArgumentException("Invalid HTTP request method name: cannot be empty");
         }
         try {
-            validateToken(name);
+            validateToken(name, "request method");
             return name;
         } catch (IllegalCharacterException e) {
             throw new IllegalArgumentException("Invalid HTTP request method name: " + name, e);

diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java
@@ -698,7 +698,9 @@ private static DecoderException newDecoderExceptionAtLine(final String message,
 
     private static DecoderException invalidHeaderName(final CharSequence name, final int parsingLine,
                                                       final IllegalCharacterException cause) {
-        throw new StacklessDecoderException("Invalid header name in line " + (parsingLine - 1) + ": " + name, cause);
+        final String msg = cause.getMessage();
+        throw new StacklessDecoderException(
+                "Invalid header name in line " + (parsingLine - 1) + ": " + (msg != null ? msg : name), cause);
     }
 
     private static DecoderException invalidHeaderValue(final CharSequence name, final int parsingLine,

diff --git a/...utils-internal/src/main/java/io/servicetalk/utils/internal/IllegalCharacterException.java b/...utils-internal/src/main/java/io/servicetalk/utils/internal/IllegalCharacterException.java
@@ -17,7 +17,6 @@
 
 import javax.annotation.Nullable;
 
-import static java.lang.Character.toChars;
 import static java.lang.Integer.toHexString;
 
 /**
@@ -26,6 +25,15 @@
 public final class IllegalCharacterException extends IllegalArgumentException {
     private static final long serialVersionUID = 5109746801766842145L;
 
+    /**
+     * Creates a new instance.
+     *
+     * @param message description message.
+     */
+    public IllegalCharacterException(final String message) {
+        super(message);
+    }
+
     /**
      * Creates a new instance.
      *
@@ -50,7 +58,7 @@ private static String message(final byte value, @Nullable final String expected)
         final int codePoint = value & 0xff;
         final StringBuilder sb = new StringBuilder(expected == null ? 10 : 23 + expected.length())
                 .append('\'')
-                .append(toChars(codePoint))
+                .append((char) codePoint)
                 .append("' (0x")
                 .append(toHexString(0x100 | codePoint), 1, 3);  // to 2 digit hex number
         return (expected == null ? sb.append(')') : sb.append("), expected [").append(expected).append(']')).toString();