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

[Sergio Durigan Junior]

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.

[Sergio Durigan Junior (sergiodj)]

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

[Bryce Harrington (bryce)]

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

[Sergio Durigan Junior (sergiodj)]

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

diff --git a/debian/changelog b/debian/changelog
index 55a4ee4..0717e3c 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+apache2 (2.4.41-4ubuntu3.11) focal; urgency=medium
+
+  * d/p/mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch:
+    Don't send GOAWAY too early on new connections when
+    MaxRequestsPerChild has been reached.  (LP: #1969629)
+
+ -- Sergio Durigan Junior <[e-mail]>  Tue, 26 Apr 2022 14:02:11 -0400
+
 apache2 (2.4.41-4ubuntu3.10) focal-security; urgency=medium
 
   * SECURITY UPDATE: OOB read in mod_lua via crafted request body
diff --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
 new file mode 100644
index 0000000..12cf833
--- /dev/null
+++ b/debian/patches/mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch
@@ -0,0 +1,61 @@
+From: Graham Leggett <[e-mail]>
+Date: Mon, 13 Dec 2021 10:33:48 +0000
+Subject: mod_http2: Don't send GOAWAY too early when MaxRequestsPerChild is
+ reached
+
+Backport:
+
+  *) mod_http2: fixes PR65731 and https://github.com/icing/mod_h2/issues/212
+     trunk patch: na, fixed on 2.4.x source base
+     backport PR: https://github.com/apache/httpd/pull/281
+     +1: icing, minfrin, ylavic
+
+git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1895869 13f79535-47bb-0310-9956-ffa450edef68
+
+Origin: backport, https://github.com/apache/httpd/commit/c1e16a66718d724feee75322cfef1a96794f00ce
+Bug: https://bz.apache.org/bugzilla/show_bug.cgi?id=65731
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/focal/+source/apache2/+bug/1969629
+---
+ modules/http2/h2_session.c | 15 ++++++++++++---
+ 1 file changed, 12 insertions(+), 3 deletions(-)
+
+diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
+index 2b773c1..d078bf7 100644
+--- a/modules/http2/h2_session.c
++++ b/modules/http2/h2_session.c
+@@ -275,7 +275,7 @@ static int on_begin_headers_cb(nghttp2_session *ngh2,
+                                const nghttp2_frame *frame, void *userp)
+ {
+     h2_session *session = (h2_session *)userp;
+-    h2_stream *s;
++    h2_stream *s = NULL;
+     
+     /* We may see HEADERs at the start of a stream or after all DATA
+      * streams to carry trailers. */
+@@ -284,7 +284,7 @@ static int on_begin_headers_cb(nghttp2_session *ngh2,
+     if (s) {
+         /* nop */
+     }
+-    else {
++    else if (session->local.accepting) {
+         s = h2_session_open_stream(userp, frame->hd.stream_id, 0);
+     }
+     return s? 0 : NGHTTP2_ERR_START_STREAM_NOT_ALLOWED;
+@@ -2108,7 +2108,16 @@ apr_status_t h2_session_process(h2_session *session, int async)
+         now = apr_time_now();
+         session->have_read = session->have_written = 0;
+ 
+-        if (session->local.accepting 
++        /* PR65731: we may get a new connection to process while the
++         * MPM already is stopping. For example due to having reached
++         * MaxRequestsPerChild limit.
++         * Since this is supposed to handle things gracefully, we need to:
++         * a) fully initialize the session before GOAWAYing
++         * b) give the client the chance to submit at least one request
++         */
++        if (session->state != H2_SESSION_ST_INIT /* no longer intializing */
++            && session->local.accepted_max > 0   /* have gotten at least one stream */
++            && session->local.accepting          /* have not already locally shut down */
+             && !ap_mpm_query(AP_MPMQ_MPM_STATE, &mpm_state)) {
+             if (mpm_state == AP_MPMQ_STOPPING) {
+                 dispatch_event(session, H2_SESSION_EV_MPM_STOPPING, 0, NULL);
diff --git a/debian/patches/series b/debian/patches/series
index 40d52bf..c2c0476 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -46,3 +46,4 @@ CVE-2022-22720.patch
 CVE-2022-22721.patch
 CVE-2022-23943-1.patch
 CVE-2022-23943-2.patch
+mod_http2-Don-t-send-GOAWAY-too-early-when-MaxReques.patch