Repository: netty/netty (Java)

Issue:
HttpObjectDecoder.getChunkedSize trips over whitespace
### Expected behavior
the chunked size is calculated correctly

### Actual behavior
java.lang.NumberFormatException is thrown

### Steps to reproduce

See code below.

Please be aware that I'm not at all familiar with the netty code. Added a few comments in the code below.

### Minimal yet complete reproducer code (or URL to code)

```
import io.netty.util.internal.StringUtil;
import java.nio.charset.StandardCharsets;

public class JavaTester {

     private static int getChunkSizeNewFixed(byte[] hex, int start, int length) {
        // byte[] is produced by LineParse::parseLine that already skip ISO CTRL and Whitespace chars
        // COMMENT: ^ I could see handling for control chars but not whitespaces ^, but maybe I'm missing something.
        int result = 0;
        for (int i = 0; i < length; i++) {
            final int digit = StringUtil.decodeHexNibble(hex[start + i]);
            if (digit == -1) {
                // COMMENT: Until version 4.1.87.final the whitespace and control char checks where in there
                if (hex[start + i] == ';' || Character.isWhitespace(hex[start + i])) {
                    return result;
                }
                throw new NumberFormatException();
            }
            result *= 16;
            result += digit;
        }
        return result;
    }

    private static int getChunkSizeNew(byte[] hex, int start, int length) {
        // byte[] is produced by LineParse::parseLine that already skip ISO CTRL and Whitespace chars
        int result = 0;
        for (int i = 0; i < length; i++) {
            final int digit = StringUtil.decodeHexNibble(hex[start + i]);
            if (digit == -1) {
                // uncommon path
                if (hex[start + i] == ';') {
                    return result;
                }
                throw new NumberFormatException();
            }
            result *= 16;
            result += digit;
        }
        return result;
    }

    public static void main(String... args) {
        String toTest = "BDC Cookie: NSC_wTfswfs_c2cjoufhsbujpo=ffffffff099b5f1f45525d5f4f58455e445a4a42378b;expires=Mon, 13-Mar-2023 14:16:06 GMT;path=/;secure;httponly";
        System.out.println(getChunkSizeNewFixed(toTest.getBytes(StandardCharsets.UTF_8), 0, 4));
        System.out.println(getChunkSizeNew(toTest.getBytes(StandardCharsets.UTF_8), 0, 4)); <-- throws NumberFormatException
    }
}
```


### Netty version

from 4.1.88 onwards.

### JVM version (e.g. `java -version`)
openjdk version "1.8.0_292"
OpenJDK Runtime Environment (AdoptOpenJDK)(build 1.8.0_292-b10)


### OS version (e.g. `uname -a`)
MacOS and Ubuntu



Hints:
@franz1981 PTAL... I think this might be related to a change of yours 
The `StringUtil.decodeHexNibble(hex[0 + 3])` call will be `StringUtil.decodeHexNibble(32)` which returns `-1`. Forgot to mention that above.
mmm I don't follow it sorry...
@lars-n are you able to make this to be triggered by actual HTTP data?
The comment I've left for myself of the future (i.e. today)
```
byte[] is produced by LineParse::parseLine that already skip ISO CTRL and Whitespace chars
```
means that there's no need to check for whitespaces because are already checked upfront
i.e. https://github.com/netty/netty/blob/3de03282d558d7b7e2c282b31db73335f0f498b1/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java#L1123
that's been used by `HttpObjectDecoder::skipControlChars` and is populated by both white spaces and CTRL chars
I'm beginning to wonder if the server I'm querying is returning crap data. Maybe pause for now while I'll try to verify that.
What I've noticed is that we receive two types of responses from that server (output from curl). The failing one is the first which is `chunked`.

Fails:

```
< HTTP/1.1 200 OK
< Transfer-Encoding: chunked
< Content-Type: text/xml; charset=UTF-8
< Server: Microsoft-IIS/10.0
< X-Powered-By: ASP.NET
< Date: Mon, 13 Mar 2023 16:47:44 GMT
< Set-Cookie: NSC_wTfswfs_c2cjoufhsbujpo=ffffffff099b5f1f45525d5f4f58455e445a4a42378b;expires=Mon, 13-Mar-2023 16:58:25 GMT;path=/;secure;httponly
```

This works (because it won't go via the getChunkedSize method):

```
< HTTP/1.1 200 OK
< Content-Type: text/xml; charset=UTF-8
< Server: Microsoft-IIS/10.0
< X-Powered-By: ASP.NET
< Date: Mon, 13 Mar 2023 16:48:04 GMT
< Content-Length: 3819
< Set-Cookie: NSC_wTfswfs_c2cjoufhsbujpo=ffffffff099b5f1f45525d5f4f58455e445a4a42378b;expires=Mon, 13-Mar-2023 16:58:45 GMT;path=/;secure;httponly
``` 

The string `"BDC Cookie: NSC_wTfswfs_c2cjoufhsbujpo=ffffffff099b5f1f45525d5f4f58455e445a4a42378b;expires=Mon, 13-Mar-2023 14:16:06 GMT;path=/;secure;httponly";` I used in the example above is the actual data from reading a chunked response like the ones above.
@franz1981 , had a look at the code around `SKIP_CONTROL_CHARS_BYTES` you mentioned. The case where is breaks is for `32` which, correct me if I'm wrong, would not be skipped by that code? I think `32` is a whitespace in ASCII. 

In my example `BDC Cook` the whitespace's value when reading `hex[start+i] ` is `32`.

Maybe as some additional input:

```
byte[] data = new byte[] {57, 32, 57}; // data like this ends up in the getChunkedSize method, can't tell why it ends up there
System.out.println(new String(data)); // "9 9"
System.out.println(Character.isWhitespace(data[1])); // true, this was used in the previous implementation of the getChunkedSize method
System.out.println(StringUtil.decodeHexNibble(data[1])); // -1
```
Yeah, the part I don't understand is the skipping, tbh, that should have worked. Will take a look 

Patch:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java
@@ -799,16 +799,41 @@ private LastHttpContent readTrailingHeaders(ByteBuf buffer) {
     protected abstract HttpMessage createMessage(String[] initialLine) throws Exception;
     protected abstract HttpMessage createInvalidMessage();
 
+    /**
+     * It skips any whitespace char and return the number of skipped bytes.
+     */
+    private static int skipWhiteSpaces(byte[] hex, int start, int length) {
+        for (int i = 0; i < length; i++) {
+            if (!isWhitespace(hex[start + i])) {
+                return i;
+            }
+        }
+        return length;
+    }
+
     private static int getChunkSize(byte[] hex, int start, int length) {
-        // byte[] is produced by LineParse::parseLine that already skip ISO CTRL and Whitespace chars
+        // trim the leading bytes if white spaces, if any
+        final int skipped = skipWhiteSpaces(hex, start, length);
+        if (skipped == length) {
+            // empty case
+            throw new NumberFormatException();
+        }
+        start += skipped;
+        length -= skipped;
         int result = 0;
         for (int i = 0; i < length; i++) {
             final int digit = StringUtil.decodeHexNibble(hex[start + i]);
             if (digit == -1) {
                 // uncommon path
-                if (hex[start + i] == ';') {
+                final byte b = hex[start + i];
+                if (b == ';' || isControlOrWhitespaceAsciiChar(b)) {
+                    if (i == 0) {
+                        // empty case
+                        throw new NumberFormatException();
+                    }
                     return result;
                 }
+                // non-hex char fail-fast path
                 throw new NumberFormatException();
             }
             result *= 16;
@@ -1127,4 +1152,8 @@ public boolean process(byte value) {
             return ISO_CONTROL_OR_WHITESPACE[128 + value];
         }
     };
+
+    private static boolean isControlOrWhitespaceAsciiChar(byte b) {
+        return ISO_CONTROL_OR_WHITESPACE[128 + b];
+    }
 }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java
@@ -147,6 +147,118 @@ public void testResponseChunked() {
         assertNull(ch.readInbound());
     }
 
+    @Test
+    public void testResponseChunkedWithValidUncommonPatterns() {
+        EmbeddedChannel ch = new EmbeddedChannel(new HttpResponseDecoder());
+        ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n",
+                                              CharsetUtil.US_ASCII));
+
+        HttpResponse res = ch.readInbound();
+        assertThat(res.protocolVersion(), sameInstance(HttpVersion.HTTP_1_1));
+        assertThat(res.status(), is(HttpResponseStatus.OK));
+
+        byte[] data = new byte[1];
+        for (int i = 0; i < data.length; i++) {
+            data[i] = (byte) i;
+        }
+
+        // leading whitespace, trailing whitespace
+
+        assertFalse(ch.writeInbound(Unpooled.copiedBuffer("  " + Integer.toHexString(data.length) + " \r\n",
+                                                          CharsetUtil.US_ASCII)));
+        assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
+        HttpContent content = ch.readInbound();
+        assertEquals(data.length, content.content().readableBytes());
+
+        byte[] decodedData = new byte[data.length];
+        content.content().readBytes(decodedData);
+        assertArrayEquals(data, decodedData);
+        content.release();
+
+        assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
+
+        // leading whitespace, trailing control char
+
+        assertFalse(ch.writeInbound(Unpooled.copiedBuffer("  " + Integer.toHexString(data.length) + "\0\r\n",
+                                                          CharsetUtil.US_ASCII)));
+        assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
+        content = ch.readInbound();
+        assertEquals(data.length, content.content().readableBytes());
+
+        decodedData = new byte[data.length];
+        content.content().readBytes(decodedData);
+        assertArrayEquals(data, decodedData);
+        content.release();
+
+        assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
+
+        // leading whitespace, trailing semicolon
+
+        assertFalse(ch.writeInbound(Unpooled.copiedBuffer("  " + Integer.toHexString(data.length) + ";\r\n",
+                                                          CharsetUtil.US_ASCII)));
+        assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
+        content = ch.readInbound();
+        assertEquals(data.length, content.content().readableBytes());
+
+        decodedData = new byte[data.length];
+        content.content().readBytes(decodedData);
+        assertArrayEquals(data, decodedData);
+        content.release();
+
+        assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
+
+        // Write the last chunk.
+        ch.writeInbound(Unpooled.copiedBuffer("0\r\n\r\n", CharsetUtil.US_ASCII));
+
+        // Ensure the last chunk was decoded.
+        LastHttpContent lastContent = ch.readInbound();
+        assertFalse(lastContent.content().isReadable());
+        lastContent.release();
+
+        ch.finish();
+        assertNull(ch.readInbound());
+    }
+
+    @Test
+    public void testResponseChunkedWithControlChars() {
+        EmbeddedChannel ch = new EmbeddedChannel(new HttpResponseDecoder());
+        ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n",
+                                              CharsetUtil.US_ASCII));
+
+        HttpResponse res = ch.readInbound();
+        assertThat(res.protocolVersion(), sameInstance(HttpVersion.HTTP_1_1));
+        assertThat(res.status(), is(HttpResponseStatus.OK));
+
+        byte[] data = new byte[1];
+        for (int i = 0; i < data.length; i++) {
+            data[i] = (byte) i;
+        }
+
+        assertFalse(ch.writeInbound(Unpooled.copiedBuffer("  " + Integer.toHexString(data.length) + " \r\n",
+                                                          CharsetUtil.US_ASCII)));
+        assertTrue(ch.writeInbound(Unpooled.copiedBuffer(data)));
+        HttpContent content = ch.readInbound();
+        assertEquals(data.length, content.content().readableBytes());
+
+        byte[] decodedData = new byte[data.length];
+        content.content().readBytes(decodedData);
+        assertArrayEquals(data, decodedData);
+        content.release();
+
+        assertFalse(ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII)));
+
+        // Write the last chunk.
+        ch.writeInbound(Unpooled.copiedBuffer("0\r\n\r\n", CharsetUtil.US_ASCII));
+
+        // Ensure the last chunk was decoded.
+        LastHttpContent lastContent = ch.readInbound();
+        assertFalse(lastContent.content().isReadable());
+        lastContent.release();
+
+        assertFalse(ch.finish());
+        assertNull(ch.readInbound());
+    }
+
     @Test
     public void testResponseDisallowPartialChunks() {
         HttpResponseDecoder decoder = new HttpResponseDecoder(
@@ -741,6 +853,121 @@ public void testGarbageChunk() {
         assertThat(channel.finish(), is(false));
     }
 
+    @Test
+    public void testWhiteSpaceGarbageChunk() {
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
+        String responseWithIllegalChunk =
+                "HTTP/1.1 200 OK\r\n" +
+                "Transfer-Encoding: chunked\r\n\r\n" +
+                " \r\n";
+
+        channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
+        assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
+
+        // Ensure that the decoder generates the last chunk with correct decoder result.
+        LastHttpContent invalidChunk = channel.readInbound();
+        assertThat(invalidChunk.decoderResult().isFailure(), is(true));
+        invalidChunk.release();
+
+        // And no more messages should be produced by the decoder.
+        assertThat(channel.readInbound(), is(nullValue()));
+
+        // .. even after the connection is closed.
+        assertThat(channel.finish(), is(false));
+    }
+
+    @Test
+    public void testLeadingWhiteSpacesSemiColonGarbageChunk() {
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
+        String responseWithIllegalChunk =
+                "HTTP/1.1 200 OK\r\n" +
+                "Transfer-Encoding: chunked\r\n\r\n" +
+                "  ;\r\n";
+
+        channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
+        assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
+
+        // Ensure that the decoder generates the last chunk with correct decoder result.
+        LastHttpContent invalidChunk = channel.readInbound();
+        assertThat(invalidChunk.decoderResult().isFailure(), is(true));
+        invalidChunk.release();
+
+        // And no more messages should be produced by the decoder.
+        assertThat(channel.readInbound(), is(nullValue()));
+
+        // .. even after the connection is closed.
+        assertThat(channel.finish(), is(false));
+    }
+
+    @Test
+    public void testControlCharGarbageChunk() {
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
+        String responseWithIllegalChunk =
+                "HTTP/1.1 200 OK\r\n" +
+                "Transfer-Encoding: chunked\r\n\r\n" +
+                "\0\r\n";
+
+        channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
+        assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
+
+        // Ensure that the decoder generates the last chunk with correct decoder result.
+        LastHttpContent invalidChunk = channel.readInbound();
+        assertThat(invalidChunk.decoderResult().isFailure(), is(true));
+        invalidChunk.release();
+
+        // And no more messages should be produced by the decoder.
+        assertThat(channel.readInbound(), is(nullValue()));
+
+        // .. even after the connection is closed.
+        assertThat(channel.finish(), is(false));
+    }
+
+    @Test
+    public void testLeadingWhiteSpacesControlCharGarbageChunk() {
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
+        String responseWithIllegalChunk =
+                "HTTP/1.1 200 OK\r\n" +
+                "Transfer-Encoding: chunked\r\n\r\n" +
+                "  \0\r\n";
+
+        channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
+        assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
+
+        // Ensure that the decoder generates the last chunk with correct decoder result.
+        LastHttpContent invalidChunk = channel.readInbound();
+        assertThat(invalidChunk.decoderResult().isFailure(), is(true));
+        invalidChunk.release();
+
+        // And no more messages should be produced by the decoder.
+        assertThat(channel.readInbound(), is(nullValue()));
+
+        // .. even after the connection is closed.
+        assertThat(channel.finish(), is(false));
+    }
+
+    @Test
+    public void testGarbageChunkAfterWhiteSpaces() {
+        EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());
+        String responseWithIllegalChunk =
+                "HTTP/1.1 200 OK\r\n" +
+                "Transfer-Encoding: chunked\r\n\r\n" +
+                "  12345N1 ;\r\n";
+
+        channel.writeInbound(Unpooled.copiedBuffer(responseWithIllegalChunk, CharsetUtil.US_ASCII));
+        assertThat(channel.readInbound(), is(instanceOf(HttpResponse.class)));
+
+        // Ensure that the decoder generates the last chunk with correct decoder result.
+        LastHttpContent invalidChunk = channel.readInbound();
+        assertThat(invalidChunk.decoderResult().isFailure(), is(true));
+        invalidChunk.release();
+
+        // And no more messages should be produced by the decoder.
+        assertThat(channel.readInbound(), is(nullValue()));
+
+        // .. even after the connection is closed.
+        assertThat(channel.finish(), is(false));
+    }
+
     @Test
     public void testConnectionClosedBeforeHeadersReceived() {
         EmbeddedChannel channel = new EmbeddedChannel(new HttpResponseDecoder());