Bug#64530: apt: The http method does not work correctly for HTTP/1.0 proxies

[amb@gedanken.demon.co.uk]

Package: apt
Version: 0.3.18
Severity: normal

The current version of the apt http method has some support for HTTP/1.0 servers
and proxies, but is not consistent.

-------------------- HttpMethod::SendReq() --------------------

   /* Build the request. We include a keep-alive header only for non-proxy
      requests. This is to tweak old http/1.0 servers that do support keep-alive
      but not HTTP/1.1 automatic keep-alive. Doing this with a proxy server 
      will glitch HTTP/1.0 proxies because they do not filter it out and 
      pass it on, HTTP/1.1 says the connection should default to keep alive
      and we expect the proxy to do this */

-------------------- HttpMethod::SendReq() --------------------

This is good, it shows that the workings of HTTP/1.0 and HTTP/1.1 are recognised
and that HTTP/1.0 will close the connection.


-------------------- ServerState::HeaderLine() --------------------

   if (stringcasecmp(Tag.begin(),Tag.begin()+4,"HTTP") == 0)
   {
      // Evil servers return no version
      if (Line[4] == '/')
      {
	 if (sscanf(Line.c_str(),"HTTP/%u.%u %u %[^\n]",&Major,&Minor,
		    &Result,Code) != 4)
	    return _error->Error("The http server sent an invalid reply header");
      }
      else
      {
	 Major = 0;
	 Minor = 9;
	 if (sscanf(Line.c_str(),"HTTP %u %[^\n]",&Result,Code) != 2)
	    return _error->Error("The http server sent an invalid reply header");
      }
      
      return true;
   }      

-------------------- ServerState::HeaderLine() --------------------

This is the code that checks for the returned header from the server.  There is
no checking here if the server is HTTP/1.0 or HTTP/1.1.  I think that you need
the following two lines before the return statement.

      if(Major==0 || (Major==1 && Minor==0))
         Encoding = Closes;


-------------------- ServerState::HeaderLine() --------------------

   if (stringcasecmp(Tag,"Content-Length:") == 0)
   {
      if (Encoding == Closes)
	 Encoding = Stream;

-------------------- ServerState::HeaderLine() --------------------

Here the if statement says that if there is a "Content-Length:" header then the
connection will not close.  This is different to what is said in the comment in
HttpMethod::SendReq(), there is no reason for an HTTP/1.0 server to not send
this header.  You need to remove those two lines.


-------------------- ServerState::HeaderLine() --------------------
-------------------- ServerState::HeaderLine() --------------------

There is no code in this function to test for "Connection:" headers like
HTTP/1.1 says there might be.  A "Connection: close" from an HTTP/1.1 server
means that the connection will be closed.

I think that you need code like this:

   if (stringcasecmp(Tag,"Connection:") == 0)
   {
    if (stringcasecmp(Val,"Close") == 0)
       Encoding = Closes;
   }


Having tried these modifications on an HTTP/1.0 proxy I have noticed a much
better performance.  Previously about half of the requests to the server were
never made (checking the server logs).  I think this is because they were made
to the same connection as the previous request.  With these changes all except
one requests work.  I am still trying to debug that one since the correct amount
of data seems to have been sent, but apt reports that gzip cannot unzip it.


-- System Information
Debian Release: 2.2
Kernel Version: Linux g2 2.2.14 #1 Sun Apr 16 09:42:29 BST 2000 i586 unknown

Versions of the packages apt depends on:
ii  libc6          2.1.3-10       GNU C Library: Shared libraries and Timezone
ii  libstdc++2.10  2.95.2-10      The GNU stdc++ library