Thanks for your swift and kind response. Adam D. Barratt wrote...
> On Fri, 2021-10-22 at 09:18 +0200, Christoph Biedl wrote:
> > ## Rework the patch
> >
> > Revert the ABI break by reworking the patch to restore the previous
> > struct layout - while maintaining the purpose of the change: Storing
> > a ninth status bit. Hilko Bengen did a great job implementing this,
> > and also reported success with several tests.
> >
> This seems like the best option if we can, although I realise it does
> break from our usual desire to use a patch as-implemented in later
> versions.
>
> Do you already have a proposed new upload / debdiff?
Find a debdiff attached, there is a lengthy explanation in the patch.
Still open on my side is a *huge* round of tests, preferably all
archtectures supported in buster, especially big-endian. This may take
major parts of the weekend.
> > PS: Related, do you check autopkgtest (...)
>
> We did discuss this on IRC briefly, but for the record - there's a
> britney instance that produces "excuses" for p-u and o-p-u, including
> scheduling autopkgtests via ci.d.n. The results show up on our tracking
> pages and we (mainly Paul; thanks!) investigate any failures raised to
> determine if they resulted from the proposed update and, if so, what to
> do about that.
Follow-up question, I might have asked that before or it has already
been answered: In case a package gets an update via a stable point
release ... I think it makes sense to take this as a chance to add an
autopkgtest if not present yet?
Rationale: The tang package I maintain as well was updated via a stable
point release in January. If I had included a backported autopkgtest, we
would have learned about the current issue in http-parser in time.
Christoph
diff -Nru http-parser-2.8.1/debian/changelog http-parser-2.8.1/debian/changelog --- http-parser-2.8.1/debian/changelog 2021-06-04 20:59:56.000000000 +0200 +++ http-parser-2.8.1/debian/changelog 2021-10-22 19:02:29.000000000 +0200 @@ -1,3 +1,11 @@ +http-parser (2.8.1-1+deb10u2) NOT-FOR-RELEASE; urgency=medium + + + NOT FOR RELEASE: This is only for review and testing. + * Fix ABI breakage introduced by accident in 2.8.1-1+deb10u1. + Closes: #996460, #996939, #996997 + + -- Christoph Biedl <debian.a...@manchmal.in-ulm.de> Fri, 22 Oct 2021 19:02:29 +0200 + http-parser (2.8.1-1+deb10u1) buster; urgency=medium * Cherry-pick "Support multi-coding Transfer-Encoding". diff -Nru http-parser-2.8.1/debian/patches/fix-ABI-breakage.patch http-parser-2.8.1/debian/patches/fix-ABI-breakage.patch --- http-parser-2.8.1/debian/patches/fix-ABI-breakage.patch 1970-01-01 01:00:00.000000000 +0100 +++ http-parser-2.8.1/debian/patches/fix-ABI-breakage.patch 2021-10-22 19:02:29.000000000 +0200 @@ -0,0 +1,191 @@ +Subject: Fix ABI breakage introduced by accident in 2.8.1-1+deb10u1 +Author: Hilko Bengen <ben...@debian.org> +Date: 2021-10-22 +Bug-Debian: + https://bugs.debian.org/996460 + https://bugs.debian.org/996939 + https://bugs.debian.org/996997 +Comment: (by the http-parser maintainer) + The fix for CVE-2019-15605 introduced an ABI break by changing the + layout of struct http_parser - a change that was needed to store a + ninth bit in the "flags" field. However, this affected offsets of + fields declared as public, causing applications to break. + + To restore the previous layout while stil implementing the fix: Steal + one bit from the content_length element (the remaining 63 are more + than enough) to store the newly introduced F_TRANSFER_ENCODING flag. + Also rename the constant to F_TRANSFER_ENCODING2 as a safeguard + against applications that try to manipulate information that is by + definition private. + + Thanks a lot to Hilko Bengen for the idea, implementation, a first + round of tests and many suggestions. -CB + +--- a/http_parser.c ++++ b/http_parser.c +@@ -25,9 +25,7 @@ + #include <string.h> + #include <limits.h> + +-#ifndef ULLONG_MAX +-# define ULLONG_MAX ((uint64_t) -1) /* 2^64-1 */ +-#endif ++#define CONTENT_LENGTH_MAX (((uint64_t)-1) >> 1) + + #ifndef MIN + # define MIN(a,b) ((a) < (b) ? (a) : (b)) +@@ -724,7 +722,8 @@ + if (ch == CR || ch == LF) + break; + parser->flags = 0; +- parser->content_length = ULLONG_MAX; ++ parser->flags2 = 0; ++ parser->content_length = CONTENT_LENGTH_MAX; + + if (ch == 'H') { + UPDATE_STATE(s_res_or_resp_H); +@@ -759,7 +758,8 @@ + case s_start_res: + { + parser->flags = 0; +- parser->content_length = ULLONG_MAX; ++ parser->flags2 = 0; ++ parser->content_length = CONTENT_LENGTH_MAX; + + switch (ch) { + case 'H': +@@ -923,7 +923,8 @@ + if (ch == CR || ch == LF) + break; + parser->flags = 0; +- parser->content_length = ULLONG_MAX; ++ parser->flags2 = 0; ++ parser->content_length = CONTENT_LENGTH_MAX; + + if (UNLIKELY(!IS_ALPHA(ch))) { + SET_ERRNO(HPE_INVALID_METHOD); +@@ -1314,7 +1315,7 @@ + parser->header_state = h_general; + } else if (parser->index == sizeof(TRANSFER_ENCODING)-2) { + parser->header_state = h_transfer_encoding; +- parser->flags |= F_TRANSFER_ENCODING; ++ parser->flags2 |= F_TRANSFER_ENCODING2; + } + break; + +@@ -1528,7 +1529,7 @@ + t += ch - '0'; + + /* Overflow? Test against a conservative limit for simplicity. */ +- if (UNLIKELY((ULLONG_MAX - 10) / 10 < parser->content_length)) { ++ if (UNLIKELY((CONTENT_LENGTH_MAX - 10) / 10 < parser->content_length)) { + SET_ERRNO(HPE_INVALID_CONTENT_LENGTH); + parser->header_state = h_state; + goto error; +@@ -1765,7 +1766,7 @@ + + /* Cannot us transfer-encoding and a content-length header together + per the HTTP specification. (RFC 7230 Section 3.3.3) */ +- if ((parser->flags & F_TRANSFER_ENCODING) && ++ if ((parser->flags2 & F_TRANSFER_ENCODING2) && + (parser->flags & F_CONTENTLENGTH)) { + /* Allow it for lenient parsing as long as `Transfer-Encoding` is + * not `chunked` +@@ -1834,7 +1835,7 @@ + parser->nread = 0; + + hasBody = parser->flags & F_CHUNKED || +- (parser->content_length > 0 && parser->content_length != ULLONG_MAX); ++ (parser->content_length > 0 && parser->content_length != CONTENT_LENGTH_MAX); + if (parser->upgrade && (parser->method == HTTP_CONNECT || + (parser->flags & F_SKIPBODY) || !hasBody)) { + /* Exit, the rest of the message is in a different protocol. */ +@@ -1850,7 +1851,7 @@ + /* chunked encoding - ignore Content-Length header, + * prepare for a chunk */ + UPDATE_STATE(s_chunk_size_start); +- } else if (parser->flags & F_TRANSFER_ENCODING) { ++ } else if (parser->flags2 & F_TRANSFER_ENCODING2) { + if (parser->type == HTTP_REQUEST && !lenient) { + /* RFC 7230 3.3.3 */ + +@@ -1877,7 +1878,7 @@ + /* Content-Length header given but zero: Content-Length: 0\r\n */ + UPDATE_STATE(NEW_MESSAGE()); + CALLBACK_NOTIFY(message_complete); +- } else if (parser->content_length != ULLONG_MAX) { ++ } else if (parser->content_length != CONTENT_LENGTH_MAX) { + /* Content-Length header given and non-zero */ + UPDATE_STATE(s_body_identity); + } else { +@@ -1901,7 +1902,7 @@ + (uint64_t) ((data + len) - p)); + + assert(parser->content_length != 0 +- && parser->content_length != ULLONG_MAX); ++ && parser->content_length != CONTENT_LENGTH_MAX); + + /* The difference between advancing content_length and p is because + * the latter will automaticaly advance on the next loop iteration. +@@ -1991,7 +1992,7 @@ + t += unhex_val; + + /* Overflow? Test against a conservative limit for simplicity. */ +- if (UNLIKELY((ULLONG_MAX - 16) / 16 < parser->content_length)) { ++ if (UNLIKELY((CONTENT_LENGTH_MAX - 16) / 16 < parser->content_length)) { + SET_ERRNO(HPE_INVALID_CONTENT_LENGTH); + goto error; + } +@@ -2035,7 +2036,7 @@ + + assert(parser->flags & F_CHUNKED); + assert(parser->content_length != 0 +- && parser->content_length != ULLONG_MAX); ++ && parser->content_length != CONTENT_LENGTH_MAX); + + /* See the explanation in s_body_identity for why the content + * length and data pointers are managed this way. +@@ -2124,12 +2125,12 @@ + } + + /* RFC 7230 3.3.3, see `s_headers_almost_done` */ +- if ((parser->flags & F_TRANSFER_ENCODING) && ++ if ((parser->flags2 & F_TRANSFER_ENCODING2) && + (parser->flags & F_CHUNKED) == 0) { + return 1; + } + +- if ((parser->flags & F_CHUNKED) || parser->content_length != ULLONG_MAX) { ++ if ((parser->flags & F_CHUNKED) || parser->content_length != CONTENT_LENGTH_MAX) { + return 0; + } + +--- a/http_parser.h ++++ b/http_parser.h +@@ -225,7 +225,7 @@ + , F_UPGRADE = 1 << 5 + , F_SKIPBODY = 1 << 6 + , F_CONTENTLENGTH = 1 << 7 +- , F_TRANSFER_ENCODING = 1 << 8 ++ , F_TRANSFER_ENCODING2 = 1 << 0 /* this goes into http_parser.flags2 */ + }; + + +@@ -296,14 +296,15 @@ + struct http_parser { + /** PRIVATE **/ + unsigned int type : 2; /* enum http_parser_type */ ++ unsigned int flags : 8; /* F_* values from 'flags' enum; semi-public */ + unsigned int state : 7; /* enum state from http_parser.c */ + unsigned int header_state : 7; /* enum header_state from http_parser.c */ + unsigned int index : 7; /* index into current matcher */ + unsigned int lenient_http_headers : 1; +- unsigned int flags : 16; /* F_* values from 'flags' enum; semi-public */ + + uint32_t nread; /* # bytes read in various scenarios */ +- uint64_t content_length; /* # bytes in body (0 if no Content-Length header) */ ++ uint64_t flags2 : 1; /* one bit another flag */ ++ uint64_t content_length : 63; /* # bytes in body (0 if no Content-Length header) */ + + /** READ-ONLY **/ + unsigned short http_major; diff -Nru http-parser-2.8.1/debian/patches/series http-parser-2.8.1/debian/patches/series --- http-parser-2.8.1/debian/patches/series 2021-05-24 10:46:26.000000000 +0200 +++ http-parser-2.8.1/debian/patches/series 2021-10-22 19:00:26.000000000 +0200 @@ -1,3 +1,4 @@ improve-installation.patch 1580760635.v2.9.2-2-g7d5c99d.support-multi-coding-transfer-encoding.patch +fix-ABI-breakage.patch
signature.asc
Description: PGP signature
