Bug 65340

3: hpack decode error, headerName: device-info, value: � 
   device-info is a base64 string

The original exception stack trace is as below in Tomcat source:

java.lang.NegativeArraySizeException: -1
	at java.base/java.lang.AbstractStringBuilder.<init>(AbstractStringBuilder.java:86)
	at java.base/java.lang.StringBuilder.<init>(StringBuilder.java:112)
	at org.apache.coyote.http2.HpackDecoder.readHuffmanString(HpackDecoder.java:231)
	at org.apache.coyote.http2.HpackDecoder.readHpackString(HpackDecoder.java:221)
	at org.apache.coyote.http2.HpackDecoder.decode(HpackDecoder.java:131)
	at org.apache.coyote.http2.Http2Parser.readHeaderPayload(Http2Parser.java:486)
	at org.apache.coyote.http2.Http2Parser.readHeadersFrame(Http2Parser.java:270)
	at org.apache.coyote.http2.Http2AsyncParser$FrameCompletionHandler.completed(Http2AsyncParser.java:251)
	at org.apache.coyote.http2.Http2AsyncParser$FrameCompletionHandler.completed(Http2AsyncParser.java:164)
	at org.apache.tomcat.util.net.SocketWrapperBase$VectoredIOCompletionHandler.completed(SocketWrapperBase.java:1089)
	at org.apache.tomcat.util.net.NioEndpoint$NioSocketWrapper$NioOperationState.run(NioEndpoint.java:1621)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
	at java.base/java.lang.Thread.run(Thread.java:829)

I'd like to know if this is a Tomcat decoding bug or a Jetty encoding bug. Please re-create the issue over h2c and provide a tcpdump of the failed request. Until there is evidence this is a Tomcat bug, I am marking this as an enhancement.

Additional debug logging is always useful. Proposed changes should be provided in diff -u format.

The proposed change is hard to read because it isn't provided as a patch but I note the following:
- patch should use Tomcat's logging framework
- patch should use Tomcat's StringManager
- no need to wrap caught exception - just catch RuntimeException and re-throw
- I'm not sure (I'd need to check the HPACK spec) if conversion to string is appropriate in all cases. It may be better to log the bytes (See HexUtils.toHexString(byte[])

this is issue happen on prod env, It diffcult to reproduce.
some request is ok, but some request is error;
it is very difficult to reproduce, and tcpdump also difficult.

the purpose of submitting this question is want to get some help for tomcat dev team, some suggestion is fine 😄

Mark,  I can't tell either if this is Jetty encoding or Tomcat decoding.

If you want to write a test to do some jetty encodes and tomcate decodes, then if you have a maven dependency on org.eclipse.jetty.http2:http2-hpack:jar
the following code shows how to do jetty encoding/decoding:

    @Test
    public void encodeDecodeTest() throws Exception
    {
        HpackEncoder encoder = new HpackEncoder();
        HpackDecoder decoder = new HpackDecoder(4096, 8192);

        HttpFields fields = new HttpFields();
        fields.add(HttpHeader.CONTENT_TYPE, "text/html");
        fields.add(HttpHeader.CONTENT_LENGTH, "1024");

        MetaData.Request request = new MetaData.Request("POST", new HttpURI("/test"), HttpVersion.HTTP_2, fields);

        ByteBuffer buffer = BufferUtil.allocateDirect(16 * 1024);
        BufferUtil.clearToFill(buffer);
        encoder.encode(buffer, request);
        BufferUtil.flipToFlush(buffer, 0);

        MetaData.Request requestReceived = (MetaData.Request)decoder.decode(buffer);

        System.err.println(requestReceived);
        requestReceived.getFields().stream().forEach(System.err::println);

        MetaData.Response response = new MetaData.Response(HttpVersion.HTTP_2, 200, fields);

        BufferUtil.clearToFill(buffer);
        encoder.encode(buffer, response);
        BufferUtil.flipToFlush(buffer, 0);

        MetaData.Response responseReceived = (Response)decoder.decode(buffer);

        System.err.println(responseReceived);
        responseReceived.getFields().stream().forEach(System.err::println);
    }

I found some information. Can you give me some answers?
1. If my header size is very big. Its length is bigger than 1024 after huffman encoding, the header will not be got.
2. The value of the variable "length" is -1 in org.apache.coyote.http2.HpackDecoder#readHpackString, why the following logic don't process it?

(In reply to Thomas from comment #6)
> I found some information. Can you give me some answers?
> 1. If my header size is very big. Its length is bigger than 1024 after
> huffman encoding, the header will not be got.  The value of headerReadBuffer.remaining() is not 0, when the header is not the first one.
    protected void readHeaderPayload(int streamId, int payloadSize, ByteBuffer buffer)
            throws Http2Exception, IOException {

        if (log.isDebugEnabled()) {
            log.debug(sm.getString("http2Parser.processFrameHeaders.payload", connectionId,
                    Integer.valueOf(streamId), Integer.valueOf(payloadSize)));
        }

        int remaining = payloadSize;

        while (remaining > 0) {
            if (headerReadBuffer.remaining() == 0) {
                // Buffer needs expansion
                int newSize;
                if (headerReadBuffer.capacity() < payloadSize) {
                    // First step, expand to the current payload. That should
                    // cover most cases.
                    newSize = payloadSize;
                } else {
                    // Header must be spread over multiple frames. Keep doubling
                    // buffer size until the header can be read.
                    newSize = headerReadBuffer.capacity() * 2;
                }
                headerReadBuffer = ByteBufferUtils.expand(headerReadBuffer, newSize);
            }
            int toRead = Math.min(headerReadBuffer.remaining(), remaining);
            // headerReadBuffer in write mode
            if (buffer == null) {
                input.fill(true, headerReadBuffer, toRead);
            } else {
                int oldLimit = buffer.limit();
                buffer.limit(buffer.position() + toRead);
                headerReadBuffer.put(buffer);
                buffer.limit(oldLimit);
            }
            // switch to read mode
            headerReadBuffer.flip();
            try {
                hpackDecoder.decode(headerReadBuffer);
            } catch (HpackException hpe) {
                throw new ConnectionException(
                        sm.getString("http2Parser.processFrameHeaders.decodingFailed"),
                        Http2Error.COMPRESSION_ERROR, hpe);
            }

            // switches to write mode
            headerReadBuffer.compact();
            remaining -= toRead;

            if (hpackDecoder.isHeaderCountExceeded()) {
                StreamException headerException = new StreamException(sm.getString(
                        "http2Parser.headerLimitCount", connectionId, Integer.valueOf(streamId)),
                        Http2Error.ENHANCE_YOUR_CALM, streamId);
                hpackDecoder.getHeaderEmitter().setHeaderException(headerException);
            }

            if (hpackDecoder.isHeaderSizeExceeded(headerReadBuffer.position())) {
                StreamException headerException = new StreamException(sm.getString(
                        "http2Parser.headerLimitSize", connectionId, Integer.valueOf(streamId)),
                        Http2Error.ENHANCE_YOUR_CALM, streamId);
                hpackDecoder.getHeaderEmitter().setHeaderException(headerException);
            }

            if (hpackDecoder.isHeaderSwallowSizeExceeded(headerReadBuffer.position())) {
                throw new ConnectionException(sm.getString("http2Parser.headerLimitSize",
                        connectionId, Integer.valueOf(streamId)), Http2Error.ENHANCE_YOUR_CALM);
            }
        }
    }

> 2. The value of the variable "length" is -1 in
> org.apache.coyote.http2.HpackDecoder#readHpackString, why the following
> logic don't process it?

    private String readHpackString(ByteBuffer buffer) throws HpackException {
        if (!buffer.hasRemaining()) {
            return null;
        }
        byte data = buffer.get(buffer.position());

        int length = Hpack.decodeInteger(buffer, 7);
        if (buffer.remaining() < length) {
            return null;
        }
        boolean huffman = (data & 0b10000000) != 0;
        if (huffman) {
            return readHuffmanString(length, buffer);
        }
        StringBuilder stringBuilder = new StringBuilder(length);
        for (int i = 0; i < length; ++i) {
            stringBuilder.append((char) buffer.get());
        }
        return stringBuilder.toString();
    }

(In reply to gregw@webtide.com from comment #5)
> Mark,  I can't tell either if this is Jetty encoding or Tomcat decoding.
> 
> If you want to write a test to do some jetty encodes and tomcate decodes,
> then if you have a maven dependency on
> org.eclipse.jetty.http2:http2-hpack:jar
> the following code shows how to do jetty encoding/decoding:
> 
>     @Test
>     public void encodeDecodeTest() throws Exception
>     {
>         HpackEncoder encoder = new HpackEncoder();
>         HpackDecoder decoder = new HpackDecoder(4096, 8192);
> 
>         HttpFields fields = new HttpFields();
>         fields.add(HttpHeader.CONTENT_TYPE, "text/html");
>         fields.add(HttpHeader.CONTENT_LENGTH, "1024");
> 
>         MetaData.Request request = new MetaData.Request("POST", new
> HttpURI("/test"), HttpVersion.HTTP_2, fields);
> 
>         ByteBuffer buffer = BufferUtil.allocateDirect(16 * 1024);
>         BufferUtil.clearToFill(buffer);
>         encoder.encode(buffer, request);
>         BufferUtil.flipToFlush(buffer, 0);
> 
>         MetaData.Request requestReceived =
> (MetaData.Request)decoder.decode(buffer);
> 
>         System.err.println(requestReceived);
>         requestReceived.getFields().stream().forEach(System.err::println);
> 
>         MetaData.Response response = new
> MetaData.Response(HttpVersion.HTTP_2, 200, fields);
> 
>         BufferUtil.clearToFill(buffer);
>         encoder.encode(buffer, response);
>         BufferUtil.flipToFlush(buffer, 0);
> 
>         MetaData.Response responseReceived =
> (Response)decoder.decode(buffer);
> 
>         System.err.println(responseReceived);
>         responseReceived.getFields().stream().forEach(System.err::println);
>     }


I have test your code, It is ok

but we do something,and found more information，when the header is larger, it appear more frequently；

we have done something:

1: when we not index and not hufman header in jetty and the header is larger than 1k, tomcat can not processs the header;

2: as thomas's reply, in javadoc in org.apache.coyote.http2.Hpack.decodeInteger(ByteBuffer, int), it can return -1, why tomcat do not process -1 to throw some more detail exception, i think it is not make sense

public class Constants {
    static final int DEFAULT_HEADER_READ_BUFFER_SIZE = 1024;
}


we found some bug for DEFAULT_HEADER_READ_BUFFER_SIZE, when one header is larger than 1024, the headerReadBuffer can not expand;

i confirm jetty encode right; we confirm from this order:
1: disable hpack index in jetty and Huffman, force header encode by Ascii
2: force header larger than 1k
3: debug tomcat decode and found can not process the header(larger than 1k)

I have reproduced the issue, please see the detail as below.
Reason description:
When the length of one header value is greater than 127 and its first prefix byte is the last one in org.apache.coyote.http2.Http2Parser#headerReadBuffer, then the return value of org.apache.coyote.http2.Hpack#decodeInteger will be -1. And then the exception "java.lang.NegativeArraySizeException: -1" is thrown in org.apache.coyote.http2.HpackDecoder#readHpackString, because the length is -1.

Reproduce steps:
example project: https://github.com/qingdaoheze/tomcat-hpack-error-reproduce
1.Start up the tomcat server through tomcat-server-gzip/src/main/java/org/sample/http2/tomcat/TomcatHttp2Main.java
2.Run the code in jetty9-client-sample/src/main/java/org/sample/jetty/client/http2/HttpClientWithHttp2Transport.java
3.The exception java.lang.NegativeArraySizeException will be thrown.

the hex string of the byte array in org.apache.coyote.http2.Http2Parser#headerReadBuffer is:
40fffb068e46638e566469c91d68653c9e2495a1b3196312c3090be0b2a249505f1297c224951297c4a89252f897c1632b4c8da7e575f68fda4eb28e4ae5e883492a28eb24bd11d68925e88925107a78f03eb47493ad08e92d1d51d3ad7e0a7322497a76acd24bd04fb3274349d251d6535bd2c0acf099b0b28fec6892529fc19124b251d472c8897a483e9594de25133d1e7e57ad7de174968474944fb32f413f5cbd04cbef4bd052f447495f132f7495fe8ff443fd257c4cf4947d24e80133d2467a4bd0e6376f1d257fa044fb2f3dd257de643d1f97a3e1e92859ec6f12747c2f0a4e881c87ede0dd2590de0fd25ec63f494e6ede0c7e92f63f829ba4a7f06ec7e92f63f837494fd8de2539bb7831e5ec63f494d234e48eb4d3258c4309232b4c8da7e575f68fda4eb28e4ae5e883492a28eb24bd11d68925e88925e83eb47493ad08e92d1d51d3ad7e0a7322497a76acd24bd04fb3274349d251d6535bd2c0acf099b0b28fec6892529fc19124b251d472c8897a483e9594de25133d1e7e57ad7de174968474944fb32f413f5cbd04cbef4bd052f447495f132f7495fe8ff443fd257c4cf4947d24e80133d2467a4bd0e6376f1d257fa044fb2f3dd257de643d1f97a3e1e92859ec6f12747c2f0a4e881c87ede0dd2590de0fd25ec63f494e6ede0c7e92f63f829ba4a7f06ec7e92f63f837494fd8de2539bb7831e5ec63f494d234e48eb4d3258c4309232b4c8da7e575f68fda4eb28e4ae5e883492a28eb24bd11d68925e88925e83eb47493ad08e92d1d51d3ad7e0a7322497a76acd24bd04fb3274349d251d6535bd2c0acf099b0b28fec6892529fc19124b251d472c8897a483e9594de25133d1e7e57ad7de174968474944fb32f413f5cbd04cbef4bd052f447495f132f7495fe8ff443fd257c4cf4947d24e80133d2467a4bd0e6376f1d257fa044fb2f3dd257de643d1f97a3e1e92859ec6f12747c2f0a4e881c87ede0dd2590de0fd25ec63f494e6ede0c7e92f63f829ba4a7f06ec7e92f63f837494fd8de2539bb7831e5ec63f494c7ede3a044fb3d25e881f4394dd3c4a7376e9298fd1979f0cbf63e9257d2514af00afbcc87a3f2f47c3d250b3d8de24e8f85e149d10390fdbc1ba4b21bc1fa4bd8c7e929cddbc18fd25ec7f0537494fe0dd8fd25ec7f06e929fb1bc4a7376f063cbd8c7e9298fdbc74089f67a4bd103e8729ba7894e6edd2531fa32f3e197ec7d24afa4a295e015f7990f47e5e8f87a4a167b1bc49d1f0bc293a20721fb783749643783f497b18fd2539bb7831fa4bd8fe0a6e929fc1bb1fa4bd8fe0dd253f637894e6ede0c797b18fd2531fb78e8113ecf497a207d0e5374f129cddba4a63f465e7c32fd8fa495f49452bc02bef321e8fcbd1f0f4942cf637893a3e178527440e43f6f06e92c86f07e92f631fa4a7fffd

Suggested solution(modify org.apache.coyote.http2.HpackDecoder#readHpackString)：
    private String readHpackString(ByteBuffer buffer) throws HpackException {
        if (!buffer.hasRemaining()) {
            return null;
        }
        byte data = buffer.get(buffer.position());

        int length = Hpack.decodeInteger(buffer, 7);
        if (buffer.remaining() < length || length == -1) {
            return null;
        }
        boolean huffman = (data & 0b10000000) != 0;
        if (huffman) {
            return readHuffmanString(length, buffer);
        }
        StringBuilder stringBuilder = new StringBuilder(length);
        for (int i = 0; i < length; ++i) {
            stringBuilder.append((char) buffer.get());
        }
        return stringBuilder.toString();
    }
the changed content is adding " || length == -1" in the following line.
if (buffer.remaining() < length || length == -1)


Summary, I suggest that:
1.Fix the bug using the alike solution mentioned above.
2.Expand the default size of org.apache.coyote.http2.Http2Parser#headerReadBuffer, for example, maxHeaderSize of the server.

@Mark Thomas

This is correct, the original code has the fix now. I'll do that.

The fix will be in 10.0.7, 9.0.47 and 8.5.67. Thanks.

Does this HPACK fix also address the HPACK index issue reported (at the Jetty issue tracker) against Tomcat?

https://github.com/eclipse/jetty.project/issues/6341

I think they are not the same issue. The issue you mentioned above is also submitted by me. The same time, I had submitted the same one in tomcat as follow:
https://bz.apache.org/bugzilla/show_bug.cgi?id=65350

Please help address it!