Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HttpObjectDecoder throws TooLongFrameException when it should not #3445

Closed
angxu opened this issue Feb 24, 2015 · 2 comments
Closed

HttpObjectDecoder throws TooLongFrameException when it should not #3445

angxu opened this issue Feb 24, 2015 · 2 comments
Assignees
Labels
Milestone

Comments

@angxu
Copy link

angxu commented Feb 24, 2015

Netty version: 4.0.25.Final

It seems the HttpObjectDecoder does not behave correctly when receiving large HTTP header whose data is across multiple ByteBufs. Specifically, the following method in HeaderParser does not reset its size or update buffer's readerIndex when it returns null. So the next time it's called with an accumulated buffer that contains the previous one, the size of the previous buffer will be counted twice.

Is this a bug or do I miss anything here?

public AppendableCharSequence parse(ByteBuf buffer) {
  seq.reset();
  int i = buffer.forEachByte(this);
  if (i == -1) {
    return null;
  }
  buffer.readerIndex(i + 1);
  return seq;
}
@angxu
Copy link
Author

angxu commented Mar 2, 2015

I wrote an unit test that can reproduce this bug against HEAD.

    @Test
    public void testMaxHeaderSize() {
      final int maxHeaderSize = 8192;

      final EmbeddedChannel ch = new EmbeddedChannel(new HttpResponseDecoder(4096, maxHeaderSize, maxHeaderSize));
      final char[] bytes = new char[maxHeaderSize / 2];
      Arrays.fill(bytes, 'a');

      try
      {
        ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.1 200 OK\r\n", CharsetUtil.US_ASCII));
        ch.writeInbound(Unpooled.copiedBuffer("X-A-Large-Header: ", CharsetUtil.US_ASCII));
        ch.writeInbound(Unpooled.copiedBuffer(bytes, CharsetUtil.US_ASCII)); // write 4096 bytes
        ch.writeInbound(Unpooled.copiedBuffer("\r\n\r\n", CharsetUtil.US_ASCII));
        // uncomment the following line with above 3 lines commented out, and the test shall pass.
        //ch.writeInbound(Unpooled.copiedBuffer("X-A-Large-Header: " + bytes + "\r\n\r\n", CharsetUtil.US_ASCII));
      }
      catch (Exception ex)
      {
        //pass through so that we can check DecoderResult;
      }

      HttpResponse res = (HttpResponse) ch.readInbound();
      assertTrue(res.getDecoderResult().isSuccess());
    }

Can someone please take a look? This is blocking us from adopting Netty 4 - which we are eagerly looking forward to because it looks all great otherwise.

Thanks!

@trustin trustin self-assigned this Mar 3, 2015
@trustin trustin added this to the 4.0.26.Final milestone Mar 3, 2015
@trustin trustin added the defect label Mar 3, 2015
trustin added a commit that referenced this issue Mar 3, 2015
Related: #3445

Motivation:

HttpObjectDecoder.HeaderParser does not reset its counter (the size
field) when it failed to find the end of line.  If a header is split
into multiple fragments, the counter is increased as many times as the
number of fragments, resulting an unexpected TooLongFrameException.

Modifications:

- Add test cases that reproduces the problem
- Reset the HeaderParser.size field when no EOL is found.

Result:

One less bug
@trustin
Copy link
Member

trustin commented Mar 3, 2015

@angxu #3460 should fix this problem.

trustin added a commit that referenced this issue Mar 4, 2015
Related: #3445

Motivation:

HttpObjectDecoder.HeaderParser does not reset its counter (the size
field) when it failed to find the end of line.  If a header is split
into multiple fragments, the counter is increased as many times as the
number of fragments, resulting an unexpected TooLongFrameException.

Modifications:

- Add test cases that reproduces the problem
- Reset the HeaderParser.size field when no EOL is found.

Result:

One less bug
trustin added a commit that referenced this issue Mar 4, 2015
Related: #3445

Motivation:

HttpObjectDecoder.HeaderParser does not reset its counter (the size
field) when it failed to find the end of line.  If a header is split
into multiple fragments, the counter is increased as many times as the
number of fragments, resulting an unexpected TooLongFrameException.

Modifications:

- Add test cases that reproduces the problem
- Reset the HeaderParser.size field when no EOL is found.

Result:

One less bug
trustin added a commit that referenced this issue Mar 4, 2015
Related: #3445

Motivation:

HttpObjectDecoder.HeaderParser does not reset its counter (the size
field) when it failed to find the end of line.  If a header is split
into multiple fragments, the counter is increased as many times as the
number of fragments, resulting an unexpected TooLongFrameException.

Modifications:

- Add test cases that reproduces the problem
- Reset the HeaderParser.size field when no EOL is found.

Result:

One less bug
trustin added a commit that referenced this issue Mar 4, 2015
Related: #3445

Motivation:

HttpObjectDecoder.HeaderParser does not reset its counter (the size
field) when it failed to find the end of line.  If a header is split
into multiple fragments, the counter is increased as many times as the
number of fragments, resulting an unexpected TooLongFrameException.

Modifications:

- Add test cases that reproduces the problem
- Reset the HeaderParser.size field when no EOL is found.

Result:

One less bug
@angxu angxu closed this as completed Mar 4, 2015
pulllock pushed a commit to pulllock/netty that referenced this issue Oct 19, 2023
Related: netty#3445

Motivation:

HttpObjectDecoder.HeaderParser does not reset its counter (the size
field) when it failed to find the end of line.  If a header is split
into multiple fragments, the counter is increased as many times as the
number of fragments, resulting an unexpected TooLongFrameException.

Modifications:

- Add test cases that reproduces the problem
- Reset the HeaderParser.size field when no EOL is found.

Result:

One less bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants