The patch series introduced some new issues, which I believe to have
resolved with the following patches. Adding them here for completeness
sake.
Sebastian Döring
Senior System Administrator
Infrastructure Server and Service Operations
1&1 Telecommunication SE | Hinterm Hauptbahnhof 3 | 76137 Karlsruhe |
Deutschland
Phone: +49 721 91374 4058 | Mobil: +49 15901248623
E-Mail: [email protected] | Web: www.1und1.de <
https://www.1und1.de >
Die gesetzlichen Pflichtangaben finden Sie unter
https://unternehmen.1und1.de/unternehmen/impressum/.
Member of United Internet
Diese E-Mail kann vertrauliche und/oder gesetzlich geschützte
Informationen enthalten. Wenn Sie nicht der bestimmungsgemäße Adressat
sind oder diese E-Mail irrtümlich erhalten haben, unterrichten Sie bitte
den Absender und vernichten Sie diese E-Mail. Anderen als dem
bestimmungsgemäßen Adressaten ist untersagt, diese E-Mail zu speichern,
weiterzuleiten oder ihren Inhalt auf welche Weise auch immer zu verwenden.
This e-mail may contain confidential and/or privileged information. If
you are not the intended recipient of this e-mail, you are hereby
notified that saving, distribution or use of the content of this e-mail
in any way is prohibited. If you have received this e-mail in error,
please notify the sender and delete the e-mail.
On 4/15/26 12:39 PM, Debian Bug Tracking System wrote:
Thank you for filing a new Bug report with Debian.
You can follow progress on this Bug here: 1133890:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1133890.
This is an automatically generated reply to let you know your message
has been received.
Your message is being forwarded to the package maintainers and other
interested parties for their attention; they will reply in due course.
As you requested using X-Debbugs-CC, your message was also forwarded to
[email protected], [email protected]
(after having been given a Bug report number, if it did not have one).
Your message has been sent to the package maintainer(s):
[email protected]
If you wish to submit further information on this problem, please
send it to [email protected].
Please do not send mail to [email protected] unless you wish
to report a problem with the Bug-tracking system.
From 9691d489b2b56516370c5685df8806d14fc001f7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20D=C3=B6ring?= <[email protected]>
Date: Mon, 6 Jul 2026 13:06:00 +0200
Subject: [PATCH] Bug 5: Fix stuck clients on failed upstream SSL connection
(HIGH SEVERITY)
---
src/tcpconnect.cc | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/src/tcpconnect.cc b/src/tcpconnect.cc
index a58e1a9..1ec4364 100644
--- a/src/tcpconnect.cc
+++ b/src/tcpconnect.cc
@@ -80,14 +80,19 @@ void tcpconnect::Disconnect()
nDisconCount.fetch_add(m_conFd >=0);
#endif
+ // Must mark file faulty BEFORE closing connections so that any client
+ // blocked waiting on an in-progress download is unblocked immediately
+ // and receives a fault signal rather than hanging indefinitely.
+ KillLastFile();
+
#ifdef HAVE_SSL
+ if (m_bio && m_ssl)
+ SSL_shutdown(m_ssl);
if(m_bio)
BIO_free_all(m_bio), m_bio=nullptr;
m_ssl = nullptr;
#endif
- m_lastFile.reset();
-
termsocket_quick(m_conFd);
}
acmutex spareConPoolMx;
--
2.47.3
From 2f3d1f7ad0f9875a24fbe13d29c9be05428ae501 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20D=C3=B6ring?= <[email protected]>
Date: Wed, 8 Jul 2026 10:38:54 +0000
Subject: [PATCH] Rework Bug 5: fix stuck clients without corrupting cache
state
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Bug 5 (commit 038b989) fixed stuck apt-get clients on SSL connection
failures by calling KillLastFile() from Disconnect(). However,
KillLastFile() -> MarkFaulty(false) -> DlSetError({...}, TRUNCATE)
introduced a cache corruption bug:
1. SafeOpenOutFile() writes a valid .head file (Content-Length,
Last-Modified) to disk when the download starts.
2. SSL connection drops -> ~tcpconnect() -> Disconnect()
-> KillLastFile() -> MarkFaulty(false) -> DlSetError(TRUNCATE)
3. TRUNCATE mode queues file+head cleanup for the destructor, but
the destructor is deferred until the last shared_ptr reference
drops. The fileitem may still be referenced by the registry,
the prolonged-lifetime queue, or an in-flight client request.
4. During this deferred window, the on-disk .head contains valid-
looking but now-inconsistent metadata (announces a file size
that does not match the incomplete data file).
5. Next apt-get update reads the stale .head, sends an If-Range/
Range probe based on the stale Content-Length and Last-Modified.
The upstream returns a 206 with a Content-Range that does not
match the expected position -> "503 Server reports unexpected
range". Deleting the cached InRelease + InRelease.head manually
fixes it because it forces a clean re-download.
Additionally, KillLastFile() was called unconditionally from
Disconnect(), meaning it would MarkFaulty() even on files that had
already completed successfully — Disconnect() is called from
~tcpconnect() which fires on every connection teardown, not just
error paths.
The fix replaces KillLastFile() in Disconnect() with a new
NotifyConnectionLost() method on fileitem that:
- Only acts when m_status < FIST_COMPLETE (in-progress transfers).
Completed or already-errored files are never touched.
- Uses EDestroyMode::KEEP instead of TRUNCATE. This preserves
cache files as-is — the .head keeps its original metadata,
the data file keeps whatever was downloaded. No deferred
truncation, no stale state.
- Still calls DlSetError() + notifyAll(), which immediately
unblocks any client waiting in AwaitSendableState().
Retry behavior and known limitation:
Because NotifyConnectionLost() sets FIST_DLERROR, and
CheckAndSaveHeader() rejects new responses when fiStatus >=
FIST_COMPLETE (DLERROR > COMPLETE in the enum), dlcon cannot
restart the download within the same job cycle. The job is
marked broken and the client receives a 503. The next apt-get
request creates a fresh fileitem that reads the preserved .head
and resumes cleanly. This matches the behavior of the original
Bug 5 patch and was confirmed acceptable in production.
Upstream (without any fix) does not touch the fileitem from
Disconnect(), so the status stays at FIST_DLRECEIVING and dlcon
can retry transparently. This patch sacrifices that transparent
retry to prevent client hangs. Fully restoring transparent retry
would require a new FiStatus enum value (e.g. FIST_DLCONNLOST
between FIST_DLRECEIVING and FIST_COMPLETE) and teaching
CheckAndSaveHeader / DlStarted to treat it as resumable — a
deeper state machine change outside the scope of a leak-fix
patch series.
The SSL teardown improvements from Bug 5 (SSL_shutdown before
BIO_free_all, m_ssl tracking) are retained — they are independent
of the fileitem lifecycle and correct on their own.
Files changed:
- src/fileitem.h: add public NotifyConnectionLost() declaration
- src/fileitem.cc: implement NotifyConnectionLost()
- src/tcpconnect.cc: replace KillLastFile() with conditional
NotifyConnectionLost() in Disconnect()
---
src/fileitem.cc | 10 ++++++++++
src/fileitem.h | 9 +++++++++
src/tcpconnect.cc | 10 ++++++----
3 files changed, 25 insertions(+), 4 deletions(-)
diff --git a/src/fileitem.cc b/src/fileitem.cc
index d8b019a..8d3d1a4 100644
--- a/src/fileitem.cc
+++ b/src/fileitem.cc
@@ -415,6 +415,16 @@ void fileitem::MarkFaulty(bool killFile)
DlSetError({500, "Bad Cache Item"}, killFile ? EDestroyMode::DELETE : EDestroyMode::TRUNCATE);
}
+void fileitem::NotifyConnectionLost()
+{
+ setLockGuard;
+ // Only act on in-progress transfers. Completed or already-errored
+ // items must not be touched — their cache state is either correct or
+ // already being handled by the regular error path.
+ if (m_status < FIST_COMPLETE)
+ DlSetError({500, "Connection lost during transfer"}, EDestroyMode::KEEP);
+}
+
ssize_t fileitem_with_storage::SendData(int out_fd, int in_fd, off_t &nSendPos, size_t count)
{
diff --git a/src/fileitem.h b/src/fileitem.h
index eca7e2b..ec9e472 100644
--- a/src/fileitem.h
+++ b/src/fileitem.h
@@ -206,6 +206,15 @@ protected:
public:
/// public proxy to DlSetError with truncation, locking!!
void MarkFaulty(bool deleteItCompletely = false);
+
+ /**
+ * @brief Notify waiting clients of a connection loss without corrupting cache state.
+ *
+ * Only acts when the transfer is still in progress (status < FIST_COMPLETE).
+ * Uses KEEP mode so cache files are preserved for resume/retry.
+ * This is the safe alternative to MarkFaulty() for use from connection teardown.
+ */
+ void NotifyConnectionLost();
/// optional method, returns raw header if needed in special implementations
virtual const std::string& GetRawResponseHeader() { return sEmptyString; }
diff --git a/src/tcpconnect.cc b/src/tcpconnect.cc
index 1ec4364..04760f8 100644
--- a/src/tcpconnect.cc
+++ b/src/tcpconnect.cc
@@ -80,10 +80,12 @@ void tcpconnect::Disconnect()
nDisconCount.fetch_add(m_conFd >=0);
#endif
- // Must mark file faulty BEFORE closing connections so that any client
- // blocked waiting on an in-progress download is unblocked immediately
- // and receives a fault signal rather than hanging indefinitely.
- KillLastFile();
+ // Notify any client waiting on an in-progress transfer that the
+ // connection was lost. Unlike KillLastFile(), this preserves cache
+ // state (KEEP mode) so the next request can resume normally.
+ if (auto p = m_lastFile.lock())
+ p->NotifyConnectionLost();
+ m_lastFile.reset();
#ifdef HAVE_SSL
if (m_bio && m_ssl)
--
2.55.0
From af45712bc9fcac8103fad52a672d5e8fca42c749 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20D=C3=B6ring?= <[email protected]>
Date: Wed, 8 Jul 2026 12:56:14 +0000
Subject: [PATCH] Fix taboo header filter in ExtractCustomHeaders (passthrough
mode)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
ExtractCustomHeaders filters client headers before forwarding them to
the upstream server in passthrough mode. The filter compares header
names against a taboo list (Host, User-Agent, Accept, etc.) using
scaseequals().
The comparison was passing it.first.data() — a const char* — which
implicitly constructs a string_view via strlen(). Since the parsed
header buffer is not null-terminated at field boundaries, strlen()
reads past the logical end of the string_view until it hits a random
null byte. This produces a string_view with incorrect length, causing
scaseequals() to always return false (length mismatch). No headers
were ever filtered.
In passthrough mode, this resulted in duplicate Host, User-Agent,
and Accept headers being sent to the upstream server:
Host: ftp.de.debian.org (added by acng)
Host: acng-server:3142 (leaked from client request)
User-Agent: Apt-Cacher-NG/3.7.5 (added by acng)
User-Agent: curl/8.14.1 (leaked from client request)
Some web servers (e.g. Apache) respond with 400 Bad Request when
they encounter duplicate Host headers.
Fix: pass it.first directly — it is already a string_view with the
correct length. This allows scaseequals() to compare lengths first
and short-circuit on mismatch, as intended.
This is a pre-existing upstream bug, not introduced by the memory
leak patch series.
---
src/header.cc | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/header.cc b/src/header.cc
index 28b29f1..aac9091 100644
--- a/src/header.cc
+++ b/src/header.cc
@@ -388,7 +388,7 @@ mstring header::ExtractCustomHeaders(string_view reqHead, bool isPassThrough)
forbidden = taboo.end() != std::find_if(taboo.begin(),
taboo.end(),
[&](cmstring &x)
- { return scaseequals(x, it.first.data()); }
+ { return scaseequals(x, it.first); }
);
if(!forbidden)
--
2.55.0
