Parse entire HTTP chunk size #396 #528

SkyTrix · 2024-03-28T14:22:10Z

Continue parsing HTTP chunk size up until Int.MaxValue so the actual size of the chunk can be logged if it exceeds the configured max chunk size.

pjfanning · 2024-03-28T14:30:09Z

http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpMessageParser.scala

          case '\r' if cursor > offset && byteChar(input, cursor + 1) == '\n' =>
            parseChunkBody(size.toInt, "", cursor + 2)
          case '\n' if cursor > offset      => parseChunkBody(size.toInt, "", cursor + 1)
          case c if CharacterClasses.WSP(c) => parseSize(cursor + 1, size) // illegal according to the spec but can happen, see issue #1812
          case c                            => failEntityStream(s"Illegal character '${escape(c)}' in chunk start")
        }
-      } else failEntityStream(s"HTTP chunk size exceeds the configured limit of ${settings.maxChunkSize} bytes")
+      } else failEntityStream(s"HTTP chunk size exceeds ${Int.MaxValue} bytes")


Can you include the text Int.MAX_VALUE in this message too? Some users may not recognise the numeric value and what it means - still possibly useful to have the actual number too.

Sure. I believe it should either be Int.MaxValue or Integer.MAX_VALUE though. Probably the latter due to Java interoperability?

Integer.MAX_VALUE might be better

I think the Scala Compiler will replace it to the acutual value.

SkyTrix · 2024-03-28T16:28:06Z

http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpMessageParser.scala

          case '\r' if cursor > offset && byteChar(input, cursor + 1) == '\n' =>
            parseChunkBody(size.toInt, "", cursor + 2)
          case '\n' if cursor > offset      => parseChunkBody(size.toInt, "", cursor + 1)
          case c if CharacterClasses.WSP(c) => parseSize(cursor + 1, size) // illegal according to the spec but can happen, see issue #1812
          case c                            => failEntityStream(s"Illegal character '${escape(c)}' in chunk start")
        }
-      } else failEntityStream(s"HTTP chunk size exceeds the configured limit of ${settings.maxChunkSize} bytes")
+      } else failEntityStream(s"HTTP chunk size exceeds Integer.MAX_VALUE (${Int.MaxValue}) bytes")


Note that this does not start parsing the actual chunk body. It only reads the size value that's written in the chunk header and bails if that's larger than Int.MaxValue. It will never attempt to parse anything larger than settings.maxChunkSize.

mdedetrich

This seems good to me, lets get someone else to also have a look

pjfanning

lgtm

He-Pin · 2024-03-28T16:59:55Z

http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpMessageParser.scala

-          case ';' if cursor > offset          => parseChunkExtensions(size.toInt, cursor + 1)()
+          case c if size > settings.maxChunkSize =>
+            failEntityStream(
+              s"HTTP chunk of $size bytes exceeds the configured limit of ${settings.maxChunkSize} bytes")


I see now, It was try decoding how big the chunk size is, and it will stop parsing how if the size already exceed MaxValue.

How about HTTP chunk size:$size exceeds the configured limit of ${settings.maxChunkSize} bytes

He-Pin

lgtm after check the code in an IDE

Roiocam

LGTM, Thanks

samueleresca

Looks good to me

samueleresca · 2024-03-29T11:53:46Z

http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpMessageParser.scala

@@ -307,17 +307,20 @@ private[http] trait HttpMessageParser[Output >: MessageOutput <: ParserOutput] {
        s"HTTP chunk extension length exceeds configured limit of ${settings.maxChunkExtLength} characters")

    @tailrec def parseSize(cursor: Int, size: Long): StateResult =
-      if (size <= settings.maxChunkSize) {
+      if (size <= Int.MaxValue) {


My personal preference (it feels more clear) would be to have:

if (size > Int.MaxValue) { // early exit here with failEntityStream } // everything else

But I don't feel is binding.

guard clauses. 👍🏻

I'm trying to keep the structure consistent with the rest of the file. I also think the above suggestion would be incorrect in the sense that everything else would still be executed after calling failEntityStream? An else block would still be required.

jxnu-liguobin · 2024-03-29T12:30:57Z

http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala

+          """400000
+            |""") should generalMultiParseTo(


Suggested change

"""400000

|""") should generalMultiParseTo(

"400000") should generalMultiParseTo(

I found that the trailing newline is significant in this and many other tests in the file.

laglangyue · 2024-03-29T13:41:02Z

http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala

@@ -531,13 +531,24 @@ abstract class RequestParserSpec(mode: String, newLine: String) extends AnyFreeS
        closeAfterResponseCompletion shouldEqual Seq(false)
      }

-      "too-large chunk size" in new Test {
+      "too-large chunk size (> Int.MaxValue)" in new Test {
        Seq(
          start,
          """1a2b3c4d5e


"1a2b3c4d5e"

SkyTrix

Could a maintainer please shed some light on what it would still take to get this merged? There are currently 6 approvals but also some comments. I don't want to invalidate approvals by making new changes if that wouldn't be required.

SkyTrix · 2024-03-31T19:07:06Z

http-core/src/main/scala/org/apache/pekko/http/impl/engine/parsing/HttpMessageParser.scala

@@ -307,17 +307,20 @@ private[http] trait HttpMessageParser[Output >: MessageOutput <: ParserOutput] {
        s"HTTP chunk extension length exceeds configured limit of ${settings.maxChunkExtLength} characters")

    @tailrec def parseSize(cursor: Int, size: Long): StateResult =
-      if (size <= settings.maxChunkSize) {
+      if (size <= Int.MaxValue) {


I'm trying to keep the structure consistent with the rest of the file. I also think the above suggestion would be incorrect in the sense that everything else would still be executed after calling failEntityStream? An else block would still be required.

SkyTrix · 2024-03-31T19:07:37Z

http-core/src/test/scala/org/apache/pekko/http/impl/engine/parsing/RequestParserSpec.scala

+          """400000
+            |""") should generalMultiParseTo(


I found that the trailing newline is significant in this and many other tests in the file.

pjfanning · 2024-03-31T19:18:55Z

Merged - thanks.

It can be refactored in new PRs.

Parse entire HTTP chunk size apache#396

91a2b45

Continue parsing HTTP chunk size up until Int.MaxValue so the actual size of the chunk can be logged if it exceeds the configured max chunk size.

pjfanning reviewed Mar 28, 2024

View reviewed changes

Add reference to Integer.MAX_VALUE in the error message

8c1c220

SkyTrix requested a review from pjfanning March 28, 2024 14:57

pjfanning requested review from jrudolph, raboof, He-Pin, mdedetrich, nvollmar, samueleresca, Roiocam and jxnu-liguobin March 28, 2024 14:58

Fix formatting

c0d4775

SkyTrix commented Mar 28, 2024

View reviewed changes

mdedetrich approved these changes Mar 28, 2024

View reviewed changes

pjfanning approved these changes Mar 28, 2024

View reviewed changes

pjfanning added this to the 1.1.0 milestone Mar 28, 2024

He-Pin reviewed Mar 28, 2024

View reviewed changes

He-Pin approved these changes Mar 28, 2024

View reviewed changes

Roiocam approved these changes Mar 29, 2024

View reviewed changes

samueleresca approved these changes Mar 29, 2024

View reviewed changes

jxnu-liguobin reviewed Mar 29, 2024

View reviewed changes

jxnu-liguobin approved these changes Mar 29, 2024

View reviewed changes

laglangyue reviewed Mar 29, 2024

View reviewed changes

SkyTrix commented Mar 31, 2024

View reviewed changes

pjfanning merged commit ab1b252 into apache:main Mar 31, 2024
10 checks passed

SkyTrix mentioned this pull request Apr 2, 2024

Improve too large chunk size test #529

Merged

pjfanning mentioned this pull request Jul 17, 2024

More descriptive failure messages in HttpMessageParser #396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse entire HTTP chunk size #396 #528

Parse entire HTTP chunk size #396 #528

SkyTrix commented Mar 28, 2024

pjfanning Mar 28, 2024

SkyTrix Mar 28, 2024

pjfanning Mar 28, 2024

He-Pin Mar 28, 2024

SkyTrix Mar 28, 2024

mdedetrich left a comment •

edited

Loading

pjfanning left a comment

He-Pin Mar 28, 2024

He-Pin Mar 28, 2024

He-Pin left a comment

Roiocam left a comment

samueleresca left a comment

samueleresca Mar 29, 2024 •

edited

Loading

Roiocam Mar 29, 2024

SkyTrix Mar 31, 2024

jxnu-liguobin Mar 29, 2024

SkyTrix Mar 31, 2024

laglangyue Mar 29, 2024

SkyTrix left a comment

SkyTrix Mar 31, 2024

SkyTrix Mar 31, 2024

pjfanning commented Mar 31, 2024

	"""400000
	\|""") should generalMultiParseTo(
	"400000") should generalMultiParseTo(

Parse entire HTTP chunk size #396 #528

Parse entire HTTP chunk size #396 #528

Conversation

SkyTrix commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdedetrich left a comment • edited Loading

Choose a reason for hiding this comment

pjfanning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

He-Pin left a comment

Choose a reason for hiding this comment

Roiocam left a comment

Choose a reason for hiding this comment

samueleresca left a comment

Choose a reason for hiding this comment

samueleresca Mar 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkyTrix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjfanning commented Mar 31, 2024

mdedetrich left a comment •

edited

Loading

samueleresca Mar 29, 2024 •

edited

Loading