Bug#465572: apt: http acquire method misbehaves on 404 from non-keepalive Server
Package: apt
Version: 0.6.46.4-0.1
Severity: important
Tags: patch
Using apt-proxy, apt-get update sometimes failed mysteriously to fetch files
known to exist.
>From the outside, this looks as follows:
APT tries to fetch a non-existent file (in our case, a DiffIndex).
You see a HTTP GET on the wire and a 404 response.
APT records the failure as Ign.
APT tries to fetch an existent URL.
You don't see anything on the wire.
APT accuses the HTTP server of having closed the connection
and records failure on the existent URL.
APT continues normally with the next URL.
>From the inside, the HTTP acquire method goes into great lengths to shoot
itself in the foot (or, more precisely, several feet) as follows:
ServerState::RunHeaders() sets Persistent = false
ServerState::HeaderLine() gets a Transfer-Encoding: chunked header.
It deduces from this, that a Body must be present, which in fact is correct,
only it might not to be the entirely right way to deduce this.
It also receives a Connection: close header.
HttpMethod::DealWithHeaders() recognizes the 404 and returns 4.
HttpMethod::Loop() calls Fail().
HttpMethod::Loop() then continues to skip the Body.
This is superflous because the connection is known to be closed afterwards.
ServerState::RunData() calls HttpMethod::Go().
HttpMethod::Go() calls CircleBuf::Read().
CircleBuf::Read() calls read() until this returns 0.
(because the server closed the connection).
HttpMethod::Go() then calls HttpMethod::ServerDie().
HttpMethod::ServerDie() calls _error and registers a closed connection.
It also closes the connection via ServerState::Close().
HttpMethod::Loop() closes the connection again because Server->Persistent
has been cleared (it doesn't check for the fd to be -1). This is harmless.
HttpMethod::Loop() then tries to re-open the connection via ServerState::Open().
ServerState::Open() calls Connect() (from methods/connect.cc).
Connect() notices a PendingError() and returns false.
The problem may be viewed as Fail() being called too early.
One could also avoid skipping the Body if the connection is already known
to be closed thereafter.
More generally, I have the impression that HttpMethod::Go() should not
call ServerDie() if Read() returns false if the connection may correctly
be closed by the server. I haven't looked into this.
The attached patch fixes our problem by moving the call to Fail() further down.
-- Package-specific info:
-- (no /etc/apt/preferences present) --
-- (/etc/apt/sources.list present, but not submitted) --
-- System Information:
Debian Release: 4.0
APT prefers stable
APT policy: (500, 'stable')
Architecture: i386 (i686)
Shell: /bin/sh linked to /bin/bash
Kernel: Linux 2.6.24.wap
Locale: LANG=de_DE@euro, LC_CTYPE=de_DE@euro (charmap=ISO-8859-15)
Versions of packages apt depends on:
ii debian-archive-keyring 2007.07.31~etch1 GnuPG archive keys of the Debian a
ii libc6 2.3.6.ds1-13etch4 GNU C Library: Shared libraries
ii libgcc1 1:4.1.1-21 GCC support library
ii libstdc++6 4.1.1-21 The GNU Standard C++ Library v3
apt recommends no packages.
-- no debconf information
--- methods/http.cc.orig 2006-12-04 15:37:36.000000000 +0100
+++ methods/http.cc 2008-02-12 17:37:52.000000000 +0100
@@ -1200,13 +1200,13 @@
// We need to flush the data, the header is like a 404 w/ error text
case 4:
{
- Fail();
-
// Send to content to dev/null
File = new FileFd("/dev/null",FileFd::WriteExists);
Server->RunData();
delete File;
File = 0;
+
+ Fail();
break;
}
Reply to: