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.axhn@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 <bengen@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
Attachment:
signature.asc
Description: PGP signature