You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "SkyTrix (via GitHub)" <gi...@apache.org> on 2024/03/28 14:22:13 UTC
[PR] Parse entire HTTP chunk size #396 [pekko-http]
SkyTrix opened a new pull request, #528:
URL: https://github.com/apache/pekko-http/pull/528
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543316251
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "SkyTrix (via GitHub)" <gi...@apache.org>.
SkyTrix commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543103986
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
Sure. I believe it should either be `Int.MaxValue` or `Integer.MAX_VALUE` though. Probably the latter due to Java interoperability?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543319890
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
Review Comment:
How about `HTTP chunk size:$size exceeds the configured limit of ${settings.maxChunkSize} bytes`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543293574
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
I did another around of check locally,it's ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "SkyTrix (via GitHub)" <gi...@apache.org>.
SkyTrix commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543270970
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1544533610
##########
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
Review Comment:
"1a2b3c4d5e"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1544424877
##########
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) {
Review Comment:
My personal preference would be to have:
```
if (size > Int.MaxValue) {
// early exit here with failEntityStream
}
// everything else
```
But I don't feel is binding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543082091
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543251863
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
I don't think we can have such a big chunk in memory, and if we do want to know how many bytes still there in the chunk, we can just logging:` there is still $n bytes remaining maybe maybe more, increase `settings.maxChunkSize` if you want to parse the full chunk`.
Parse it until the `Int.MaxValue` seems some kind of ddos for me, @jrudolph cc
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
And eg, In Netty, there is a `maxContentLength` to be configured and user can config how to response the when oversized with `handleOversizedMessage`.see : `HttpObjectAggregator`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "jxnu-liguobin (via GitHub)" <gi...@apache.org>.
jxnu-liguobin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1544457839
##########
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
|""") should generalMultiParseTo(
Right(baseRequest),
- Left(EntityStreamError(ErrorInfo("HTTP chunk size exceeds the configured limit of 1048576 bytes"))))
+ Left(EntityStreamError(ErrorInfo("HTTP chunk size exceeds Integer.MAX_VALUE (2147483647) bytes"))))
+ closeAfterResponseCompletion shouldEqual Seq(false)
+ }
+
+ "too-large chunk size" in new Test {
+ Seq(
+ start,
+ """400000
+ |""") should generalMultiParseTo(
Review Comment:
```suggestion
"400000") should generalMultiParseTo(
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543251863
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
I don't think we can have such a big chunk in memory, and if we do want to know how many bytes still there in the chunk, we can just logging:` there is still $n bytes remaining maybe maybe more, increase `settings.maxChunkSize` if you want to parse the full chunk`.
Parse it until the `Int.MaxValue` seems some kind of ddos for me, @jrudolph cc
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "samueleresca (via GitHub)" <gi...@apache.org>.
samueleresca commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1544424877
##########
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) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543280765
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
And eg, In Netty, there is a `maxContentLength` to be configured and user can config how to response the when oversized with `handleOversizedMessage`.see : `HttpObjectAggregator`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543328024
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
In netty, when oversize, it invoke a protect method `handleOversizedMessage`, maybe we can introduce something like that too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #528:
URL: https://github.com/apache/pekko-http/pull/528#issuecomment-2028881822
Merged - thanks.
It can be refactored in new PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "Roiocam (via GitHub)" <gi...@apache.org>.
Roiocam commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1544437401
##########
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) {
Review Comment:
guard clauses. 👍🏻
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543339031
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
I think the Scala Compiler will replace it to the acutual value.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543105767
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
Integer.MAX_VALUE might be better
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1543328024
##########
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) {
byteChar(input, cursor) match {
case c if CharacterClasses.HEXDIG(c) => parseSize(cursor + 1, size * 16 + CharUtils.hexValue(c))
- 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")
+ case ';' if cursor > offset => parseChunkExtensions(size.toInt, cursor + 1)()
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")
Review Comment:
In netty, when oversize, it invoke a protect method `handleOversizedMessage`, maybe we can introduce something like that too.
see: io.netty.handler.codec.http.HttpObjectAggregator#handleOversizedMessage
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "SkyTrix (via GitHub)" <gi...@apache.org>.
SkyTrix commented on code in PR #528:
URL: https://github.com/apache/pekko-http/pull/528#discussion_r1545787080
##########
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
|""") should generalMultiParseTo(
Right(baseRequest),
- Left(EntityStreamError(ErrorInfo("HTTP chunk size exceeds the configured limit of 1048576 bytes"))))
+ Left(EntityStreamError(ErrorInfo("HTTP chunk size exceeds Integer.MAX_VALUE (2147483647) bytes"))))
+ closeAfterResponseCompletion shouldEqual Seq(false)
+ }
+
+ "too-large chunk size" in new Test {
+ Seq(
+ start,
+ """400000
+ |""") should generalMultiParseTo(
Review Comment:
I found that the trailing newline is significant in this and many other tests in the file.
##########
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) {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
Re: [PR] Parse entire HTTP chunk size #396 [pekko-http]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning merged PR #528:
URL: https://github.com/apache/pekko-http/pull/528
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org