Merge ~sergiodj/ubuntu/+source/apache2:bug1969629-http2-empty-response-maxrequestsperchild-FOCAL into ubuntu/+source/apache2:ubuntu/focal-devel

Proposed by Sergio Durigan Junior
Status: Merged
Merge reported by: Sergio Durigan Junior
Merged at revision: 6ae3d9321009313f0ffd75f61b902a9b01cc19e4
Proposed branch: ~sergiodj/ubuntu/+source/apache2:bug1969629-http2-empty-response-maxrequestsperchild-FOCAL
Merge into: ubuntu/+source/apache2:ubuntu/focal-devel
Diff against target: 92 lines (+70/-0)
3 files modified
debian/changelog (+8/-0)
debian/patches/mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch (+61/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Bryce Harrington (community) Approve
Canonical Server Pending
Review via email: mp+420582@code.launchpad.net

Description of the change

This MP fixes bug #1969629.

This bug affects apache2 on Bionic, Focal and Impish. It's an issue that happens when (a) HTTP2 is being used and (b) the MaxRequestsPerChild limit is reached. In this scenario, apache2 will send a GOAWAY packet to the client too early in the connection, which will eventually lead to no data being transferred.

This bug has been reported and fixed upstream:

https://bz.apache.org/bugzilla/show_bug.cgi?id=65731
https://github.com/apache/httpd/commit/c1e16a66718d724feee75322cfef1a96794f00ce

I backported the patch above, and in the process I had to drop a few things from it:

- I dropped all the README/changelog update bits from the patch, as usual.

- I decided to drop the version bump of mod_http2. I don't think it makes sense to adjust the module version because of this patch specifically.

- None of the apache2 packages available on B/F/I have the h2_workers_graceful_shutdown function defined, so I also dropped that bit.

You can find instructions on how to reproduce the bug and test the package in the SRU template.

There's a PPA with the proposed changes here:

https://launchpad.net/~sergiodj/+archive/ubuntu/apache2/+packages

The amd64 and i386 builds are still pending on the PPA; I will post autopkgtest results here when they're done.

To post a comment you must log in.
Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

autopkgtest is OK:

autopkgtest [19:40:03]: @@@@@@@@@@@@@@@@@@@@ summary
run-test-suite PASS
duplicate-module-load PASS
htcacheclean PASS
default-mods PASS
ssl-passphrase PASS
check-http2 PASS
chroot PASS

Revision history for this message
Bryce Harrington (bryce) wrote :

Reviewing the bug filed to launchpad and upstream, looks like a straightforward problem with a well-defined solution, which makes sense to SRU.

I set up the three LXC containers and ran the test cases on all three releases, verified the error prints "BUG DETECTED", then installed the PPA and verified test case runs without exiting, as documented for the expected behavior.

Code review of the patch looks ok, and the changes dropped compared with the upstream version of the patch makes sense.

SRU text is quite well written; the test case is super paint-by-numbers and I really like how you suggested courses of action in case of regression in the where problems could occur section.

Nothing at all to complain about. LVGTM, +1.

review: Approve
Revision history for this message
Sergio Durigan Junior (sergiodj) wrote :

On Wednesday, April 27 2022, Bryce Harrington wrote:

> Review: Approve
>
> Reviewing the bug filed to launchpad and upstream, looks like a straightforward problem with a well-defined solution, which makes sense to SRU.
>
> I set up the three LXC containers and ran the test cases on all three
> releases, verified the error prints "BUG DETECTED", then installed the
> PPA and verified test case runs without exiting, as documented for the
> expected behavior.
>
> Code review of the patch looks ok, and the changes dropped compared with the upstream version of the patch makes sense.
>
> SRU text is quite well written; the test case is super
> paint-by-numbers and I really like how you suggested courses of action
> in case of regression in the where problems could occur section.
>
> Nothing at all to complain about. LVGTM, +1.

Thanks for the review, Bryce.

Uploaded:

$ dput apache2_2.4.41-4ubuntu3.11_source.changes
Trying to upload package to ubuntu
Checking signature on .changes
gpg: /home/sergio/work/apache2/apache2_2.4.41-4ubuntu3.11_source.changes: Valid signature from 106DA1C8C3CBBF14
Checking signature on .dsc
gpg: /home/sergio/work/apache2/apache2_2.4.41-4ubuntu3.11.dsc: Valid signature from 106DA1C8C3CBBF14
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading apache2_2.4.41-4ubuntu3.11.dsc: done.
  Uploading apache2_2.4.41-4ubuntu3.11.debian.tar.xz: done.
  Uploading apache2_2.4.41-4ubuntu3.11_source.buildinfo: done.
  Uploading apache2_2.4.41-4ubuntu3.11_source.changes: done.
Successfully uploaded packages.

--
Sergio
GPG key ID: E92F D0B3 6B14 F1F4 D8E0 EB2F 106D A1C8 C3CB BF14

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 55a4ee4..0717e3c 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,11 @@
6+apache2 (2.4.41-4ubuntu3.11) focal; urgency=medium
7+
8+ * d/p/mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch:
9+ Don't send GOAWAY too early on new connections when
10+ MaxRequestsPerChild has been reached. (LP: #1969629)
11+
12+ -- Sergio Durigan Junior <sergio.durigan@canonical.com> Tue, 26 Apr 2022 14:02:11 -0400
13+
14 apache2 (2.4.41-4ubuntu3.10) focal-security; urgency=medium
15
16 * SECURITY UPDATE: OOB read in mod_lua via crafted request body
17diff --git a/debian/patches/mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch b/debian/patches/mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch
18new file mode 100644
19index 0000000..12cf833
20--- /dev/null
21+++ b/debian/patches/mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch
22@@ -0,0 +1,61 @@
23+From: Graham Leggett <minfrin@apache.org>
24+Date: Mon, 13 Dec 2021 10:33:48 +0000
25+Subject: mod_http2: Don't send GOAWAY too early when MaxRequestsPerChild is
26+ reached
27+
28+Backport:
29+
30+ *) mod_http2: fixes PR65731 and https://github.com/icing/mod_h2/issues/212
31+ trunk patch: na, fixed on 2.4.x source base
32+ backport PR: https://github.com/apache/httpd/pull/281
33+ +1: icing, minfrin, ylavic
34+
35+git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1895869 13f79535-47bb-0310-9956-ffa450edef68
36+
37+Origin: backport, https://github.com/apache/httpd/commit/c1e16a66718d724feee75322cfef1a96794f00ce
38+Bug: https://bz.apache.org/bugzilla/show_bug.cgi?id=65731
39+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/focal/+source/apache2/+bug/1969629
40+---
41+ modules/http2/h2_session.c | 15 ++++++++++++---
42+ 1 file changed, 12 insertions(+), 3 deletions(-)
43+
44+diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
45+index 2b773c1..d078bf7 100644
46+--- a/modules/http2/h2_session.c
47++++ b/modules/http2/h2_session.c
48+@@ -275,7 +275,7 @@ static int on_begin_headers_cb(nghttp2_session *ngh2,
49+ const nghttp2_frame *frame, void *userp)
50+ {
51+ h2_session *session = (h2_session *)userp;
52+- h2_stream *s;
53++ h2_stream *s = NULL;
54+
55+ /* We may see HEADERs at the start of a stream or after all DATA
56+ * streams to carry trailers. */
57+@@ -284,7 +284,7 @@ static int on_begin_headers_cb(nghttp2_session *ngh2,
58+ if (s) {
59+ /* nop */
60+ }
61+- else {
62++ else if (session->local.accepting) {
63+ s = h2_session_open_stream(userp, frame->hd.stream_id, 0);
64+ }
65+ return s? 0 : NGHTTP2_ERR_START_STREAM_NOT_ALLOWED;
66+@@ -2108,7 +2108,16 @@ apr_status_t h2_session_process(h2_session *session, int async)
67+ now = apr_time_now();
68+ session->have_read = session->have_written = 0;
69+
70+- if (session->local.accepting
71++ /* PR65731: we may get a new connection to process while the
72++ * MPM already is stopping. For example due to having reached
73++ * MaxRequestsPerChild limit.
74++ * Since this is supposed to handle things gracefully, we need to:
75++ * a) fully initialize the session before GOAWAYing
76++ * b) give the client the chance to submit at least one request
77++ */
78++ if (session->state != H2_SESSION_ST_INIT /* no longer intializing */
79++ && session->local.accepted_max > 0 /* have gotten at least one stream */
80++ && session->local.accepting /* have not already locally shut down */
81+ && !ap_mpm_query(AP_MPMQ_MPM_STATE, &mpm_state)) {
82+ if (mpm_state == AP_MPMQ_STOPPING) {
83+ dispatch_event(session, H2_SESSION_EV_MPM_STOPPING, 0, NULL);
84diff --git a/debian/patches/series b/debian/patches/series
85index 40d52bf..c2c0476 100644
86--- a/debian/patches/series
87+++ b/debian/patches/series
88@@ -46,3 +46,4 @@ CVE-2022-22720.patch
89 CVE-2022-22721.patch
90 CVE-2022-23943-1.patch
91 CVE-2022-23943-2.patch
92+mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch

Subscribers

People subscribed via source and target branches