Merge ~sergiodj/ubuntu/+source/squid:memory-leak-connect-tunnel-closed into ubuntu/+source/squid:ubuntu/jammy-devel

[Sergio Durigan Junior]

This MP fixes a memory leak that's affecting squid on Jammy.

Unfortunately, it's yet another one of those bugs that's hard to reproduce locally due to its non-determinism. I couldn't reproduce it, but fortunately the reporter has been very helpful and was able not only to make the memory leak happen consistently but also to confirm that this fix I'm proposing indeed solves the problem.

You can also check upstream's discussion and lengthy investigation here: https://bugs.squid-cache.org/show_bug.cgi?id=5132

As you can see, the patch is very simple and functionally just adds a call to retryOrBail, which acts like a destructor freeing up unneeded resources.

There's a PPA with the proposed changes here:

https://launchpad.net/~sergiodj/+archive/ubuntu/squid-bug1989380-proper

autopkgtest results:

Results: (from http://autopkgtest.ubuntu.com/results/autopkgtest-jammy-sergiodj-squid-bug1989380-proper/?format=plain)
  squid @ amd64:
    19.01.23 23:21:01 Log 🗒️ ✅ Triggers: squid/5.2-1ubuntu4.3~ppa1
  squid @ arm64:
    19.01.23 23:19:07 Log 🗒️ ✅ Triggers: squid/5.2-1ubuntu4.3~ppa1
  squid @ ppc64el:
    19.01.23 23:07:15 Log 🗒️ ✅ Triggers: squid/5.2-1ubuntu4.3~ppa1
  squid @ s390x:
    19.01.23 23:03:24 Log 🗒️ ✅ Triggers: squid/5.2-1ubuntu4.3~ppa1

I retriggered the test on armhf because it failed there due to flakiness.

[Bryce Harrington (bryce)]

The fix in question is indeed simple - just a one-liner - but it does change behavior on server closure so side-effects could be worth looking for, as is noted in LP: #1989380.

The upstream bug report itemizes a very detailed analysis and identification of the flaw, and as mentioned in the SRU text it sounds like we can rely on them if there is indeed a regression. The LP bug reporter also appears actively involved so we can depend on them for helping with the verification.

I'm going to re-read the analysis just to understand it better, but otherwise everything looks good. Packaging is solid, as is the testing and SRU text. +1 for upload.

review: Approve

[git-ubuntu bot (git-ubuntu-bot)]

Approvers: sergiodj, bryce
Uploaders: sergiodj, bryce
MP auto-approved

review: Approve

[Sergio Durigan Junior (sergiodj)]

On Friday, January 20 2023, Bryce Harrington wrote:

> The fix in question is indeed simple - just a one-liner - but it does
> change behavior on server closure so side-effects could be worth
> looking for, as is noted in LP: #1989380.
>
> The upstream bug report itemizes a very detailed analysis and
> identification of the flaw, and as mentioned in the SRU text it sounds
> like we can rely on them if there is indeed a regression. The LP bug
> reporter also appears actively involved so we can depend on them for
> helping with the verification.
>
> I'm going to re-read the analysis just to understand it better, but
> otherwise everything looks good. Packaging is solid, as is the
> testing and SRU text. +1 for upload.

Thanks a lot for the review, Bryce.

Uploaded:

$ dput squid_5.2-1ubuntu4.3_source.changes
Trying to upload package to ubuntu
Checking signature on .changes
gpg: /home/sergio/work/squid/squid_5.2-1ubuntu4.3_source.changes: Valid signature from 106DA1C8C3CBBF14
Checking signature on .dsc
gpg: /home/sergio/work/squid/squid_5.2-1ubuntu4.3.dsc: Valid signature from 106DA1C8C3CBBF14
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading squid_5.2-1ubuntu4.3.dsc: done.
  Uploading squid_5.2-1ubuntu4.3.debian.tar.xz: done.
  Uploading squid_5.2-1ubuntu4.3_source.buildinfo: done.
  Uploading squid_5.2-1ubuntu4.3_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 ad43ef6..4cadc22 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+squid (5.2-1ubuntu4.3) jammy; urgency=medium
+
+  * d/p/close-tunnel-if-to-server-conn-closes-after-client.patch:
+    Close tunnel "job" after to-server client connection closes,
+    fixing memory leak. (LP: #1989380)
+
+ -- Sergio Durigan Junior <[e-mail]>  Thu, 05 Jan 2023 15:50:48 -0500
+
 squid (5.2-1ubuntu4.2) jammy-security; urgency=medium
 
   * SECURITY UPDATE: Exposure of Sensitive Information in Cache Manager
diff --git a/debian/patches/close-tunnel-if-to-server-conn-closes-after-client.patch b/debian/patches/close-tunnel-if-to-server-conn-closes-after-client.patch
new file mode 100644
index 0000000..96b77ab
--- /dev/null
+++ b/debian/patches/close-tunnel-if-to-server-conn-closes-after-client.patch
@@ -0,0 +1,45 @@
+From: Alex Rousskov <[e-mail]>
+Date: Sun, 9 Jan 2022 10:41:24 +0000
+Subject: Bug 5132: Close the tunnel if to-server conn closes after client
+ (#957)
+
+Since commit 25d2603, blind CONNECT tunnel "jobs" (and equivalent) were
+not destroyed upon a "lonely" to-server connection closure, leading to
+memory leaks. And when a from-client connection was still present at the
+time of the to-server connection closure, we did not try to reforward,
+violating the spirit of commit 25d2603 changes. Calling retryOrBail() is
+sufficient to handle both cases.
+
+Origin: upstream, https://github.com/squid-cache/squid/commit/54ad10efe146c863bdadee0d1161299ea89f5419
+Bug: https://bugs.squid-cache.org/show_bug.cgi?id=5132
+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/squid/+bug/1989380
+Reviewed-By: Sergio Durigan Junior <[e-mail]>
+Last-Update: 2023-01-20
+Applied-Upstream: 5.4
+---
+ src/tunnel.cc | 5 ++---
+ 1 file changed, 2 insertions(+), 3 deletions(-)
+
+diff --git a/src/tunnel.cc b/src/tunnel.cc
+index eb06544..159669b 100644
+--- a/src/tunnel.cc
++++ b/src/tunnel.cc
+@@ -316,6 +316,8 @@ void
+ TunnelStateData::serverClosed()
+ {
+     server.noteClosure();
++
++    retryOrBail(__FUNCTION__);
+ }
+ 
+ /// TunnelStateData::clientClosed() wrapper
+@@ -418,9 +420,6 @@ TunnelStateData::checkRetry()
+ void
+ TunnelStateData::retryOrBail(const char *context)
+ {
+-    // Since no TCP payload has been passed to client or server, we may
+-    // TCP-connect to other destinations (including alternate IPs).
+-
+     assert(!server.conn);
+ 
+     const auto *bailDescription = checkRetry();
diff --git a/debian/patches/series b/debian/patches/series
index 1c4ec9e..dd4f3e8 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -21,3 +21,4 @@ openssl3-Remove-stale-TODO-and-comment.patch
 CVE-2021-46784.patch
 CVE-2022-41317.patch
 CVE-2022-41318.patch
+close-tunnel-if-to-server-conn-closes-after-client.patch