Project

General

Profile

Bug #2854

chunked transfer encoding in request body only works for tiny chunks

Added by the_jk 9 days ago. Updated 6 days ago.

Status:
Patch Pending
Priority:
Normal
Assignee:
-
Category:
core
Target version:
Start date:
2018-01-10
Due date:
% Done:

0%

Estimated time:
Missing in 1.5.x:

Description

curl -v --data-ascii "0123456789abcdef" -H "Transfer-Encoding: chunked" 

fails with "400 - Bad Request" while
curl -v --data-ascii "0123456789abcde" -H "Transfer-Encoding: chunked" 

succeeds as the chunk is less than 16 bytes.

Bug caused by an incorrect overflow protection in connection_handle_read_post_chunked in connections-glue.c

History

#2

Updated by gstrauss 8 days ago

  • Status changed from New to Patch Pending
  • Target version changed from 1.4.x to 1.4.49

Ick. Thanks for reporting this bug. It is undefined behavior to right-shift a signed integral value.
Try this:

--- a/src/connections-glue.c
+++ b/src/connections-glue.c
@@ -132,7 +132,13 @@ static handler_t connection_handle_read_post_chunked(server *srv, connection *co
                 off_t hsz = p + 1 - (c->mem->ptr+c->offset);
                 unsigned char *s = (unsigned char *)c->mem->ptr+c->offset;
                 for (unsigned char u;(u=(unsigned char)hex2int(*s))!=0xFF;++s) {
-                    if (te_chunked > (~((off_t)-1) >> 4)) {
+                    static const off_t plim =
+                      (sizeof(off_t) == sizeof(uint64_t))
+                      ? (off_t)((uint64_t)~(uint64_t)0 >> 5)   /*INT64_MAX>>4*/
+                      : (sizeof(off_t) == sizeof(uint32_t))
+                        ? (off_t)((uint32_t)~(uint32_t)0 >> 5) /*INT32_MAX>>4*/
+                        : (off_t)((uint16_t)~(uint16_t)0 >> 5);/*INT16_MAX>>4*/
+                    if (te_chunked > plim) {
                         log_error_write(srv, __FILE__, __LINE__, "s",
                                         "chunked data size too large -> 400");
                         /* 400 Bad Request */

#3

Updated by the_jk 6 days ago

Right, my suggestion ended up comparing with UINT64_MAX>>4. Your code above works. But the conditionals in the above hurts my eyes ;) so, an alternative version that also work:

            (off_t)(1lu << (8 * sizeof(off_t) - 5)) - 1)

Tried to write a testcase to actually test overflow but couldn't figure out how to make the test conditional on sizeof(off_t)

Also available in: Atom