From 148c049150cc39f2e40894c1684dc2aefea1117e Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 12 Aug 2016 22:13:09 +0200 Subject: http(s): allow empty values for header fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It seems completely pointless from a server-POV to sent empty header fields, so most of them don't do it (simply proven by this limitation existing since day one) – but it is technically allowed by the RFC as the surounding whitespaces are optional and Github seems to like sending "X-Geo-Block-List:\r\n" since recently (bug reports in other http clients indicate July) at least sometimes as the reporter claims to have seen it on https only even through it can happen with both. Closes: 834048 --- methods/server.cc | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) (limited to 'methods') diff --git a/methods/server.cc b/methods/server.cc index 0888617b1..3f0e88457 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -101,25 +101,7 @@ bool ServerState::HeaderLine(string Line) if (Line.empty() == true) return true; - string::size_type Pos = Line.find(' '); - if (Pos == string::npos || Pos+1 > Line.length()) - { - // Blah, some servers use "connection:closes", evil. - Pos = Line.find(':'); - if (Pos == string::npos || Pos + 2 > Line.length()) - return _error->Error(_("Bad header line")); - Pos++; - } - - // Parse off any trailing spaces between the : and the next word. - string::size_type Pos2 = Pos; - while (Pos2 < Line.length() && isspace_ascii(Line[Pos2]) != 0) - Pos2++; - - string Tag = string(Line,0,Pos); - string Val = string(Line,Pos2); - - if (stringcasecmp(Tag.c_str(),Tag.c_str()+4,"HTTP") == 0) + if (Line.size() > 4 && stringcasecmp(Line.data(), Line.data()+4, "HTTP") == 0) { // Evil servers return no version if (Line[4] == '/') @@ -163,6 +145,21 @@ bool ServerState::HeaderLine(string Line) return true; } + // Blah, some servers use "connection:closes", evil. + // and some even send empty header fields… + string::size_type Pos = Line.find(':'); + if (Pos == string::npos) + return _error->Error(_("Bad header line")); + ++Pos; + + // Parse off any trailing spaces between the : and the next word. + string::size_type Pos2 = Pos; + while (Pos2 < Line.length() && isspace_ascii(Line[Pos2]) != 0) + Pos2++; + + string const Tag(Line,0,Pos); + string const Val(Line,Pos2); + if (stringcasecmp(Tag,"Content-Length:") == 0) { if (Encoding == Closes) -- cgit v1.2.3 From ebdb6f1810a20ac240b5b2192dc2e6532ff149d2 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 11 Aug 2016 16:59:13 +0200 Subject: reorganize server-states resetting in http/https MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We keep various information bits about the server around, some only effecting the currently handled file (like sizes) while others should be persistent (like pipeline detections). http used to reset all file-related manually, which is a bit silly if we already have a Reset() method – which does reset all through –, so extending it with a parameter for reuse and calling it from https too (as this was previously resetting by just creating a new state struct – it uses no value of the persistent state-keeping yet as it supports no pipelining). Gbp-Dch: Ignore --- methods/http.cc | 7 +++++++ methods/http.h | 2 +- methods/https.cc | 5 ++++- methods/server.cc | 25 +++++++++++++------------ methods/server.h | 4 +--- 5 files changed, 26 insertions(+), 17 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 0358b50cd..8d34aa6e3 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -647,6 +647,13 @@ bool HttpServerState::InitHashes(HashStringList const &ExpectedHashes) /*{{{*/ return true; } /*}}}*/ +void HttpServerState::Reset(bool const Everything) /*{{{*/ +{ + ServerState::Reset(Everything); + if (Everything) + ServerFd = -1; +} + /*}}}*/ APT_PURE Hashes * HttpServerState::GetHashes() /*{{{*/ { diff --git a/methods/http.h b/methods/http.h index 644e576f0..b7341f5f8 100644 --- a/methods/http.h +++ b/methods/http.h @@ -103,7 +103,7 @@ struct HttpServerState: public ServerState virtual bool WriteResponse(std::string const &Data) APT_OVERRIDE; public: - virtual void Reset() APT_OVERRIDE { ServerState::Reset(); ServerFd = -1; }; + virtual void Reset(bool const Everything = true) APT_OVERRIDE; virtual bool RunData(FileFd * const File) APT_OVERRIDE; virtual bool RunDataToDevNull() APT_OVERRIDE; diff --git a/methods/https.cc b/methods/https.cc index 283126f6b..c86f9407e 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -399,7 +399,10 @@ bool HttpsMethod::Fetch(FetchItem *Itm) // go for it - if the file exists, append on it File = new FileFd(Itm->DestFile, FileFd::WriteAny); - Server = CreateServerState(Itm->Uri); + if (Server == nullptr || Server->Comp(Itm->Uri) == false) + Server = CreateServerState(Itm->Uri); + else + Server->Reset(false); if (Server->InitHashes(Itm->ExpectedHashes) == false) return false; diff --git a/methods/server.cc b/methods/server.cc index 3f0e88457..d0b6ef3d7 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -46,20 +46,9 @@ time_t ServerMethod::FailTime = 0; ServerState::RunHeadersResult ServerState::RunHeaders(FileFd * const File, const std::string &Uri) { - State = Header; - + Reset(false); Owner->Status(_("Waiting for headers")); - Major = 0; - Minor = 0; - Result = 0; - TotalFileSize = 0; - JunkSize = 0; - StartPos = 0; - Encoding = Closes; - HaveContent = false; - time(&Date); - do { string Data; @@ -254,6 +243,18 @@ bool ServerState::AddPartialFileToHashes(FileFd &File) /*{{{*/ return GetHashes()->AddFD(File, StartPos); } /*}}}*/ +void ServerState::Reset(bool const Everything) /*{{{*/ +{ + Major = 0; Minor = 0; Result = 0; Code[0] = '\0'; + TotalFileSize = 0; JunkSize = 0; StartPos = 0; + Encoding = Closes; time(&Date); HaveContent = false; + State = Header; MaximumSize = 0; + if (Everything) + { + Persistent = false; Pipeline = false; PipelineAllowed = true; + } +} + /*}}}*/ // ServerMethod::DealWithHeaders - Handle the retrieved header data /*{{{*/ // --------------------------------------------------------------------- diff --git a/methods/server.h b/methods/server.h index 1d114354f..63acceae6 100644 --- a/methods/server.h +++ b/methods/server.h @@ -84,9 +84,7 @@ struct ServerState bool AddPartialFileToHashes(FileFd &File); bool Comp(URI Other) const {return Other.Host == ServerName.Host && Other.Port == ServerName.Port;}; - virtual void Reset() {Major = 0; Minor = 0; Result = 0; Code[0] = '\0'; TotalFileSize = 0; JunkSize = 0; - StartPos = 0; Encoding = Closes; time(&Date); HaveContent = false; - State = Header; Persistent = false; Pipeline = false; MaximumSize = 0; PipelineAllowed = true;}; + virtual void Reset(bool const Everything = true); virtual bool WriteResponse(std::string const &Data) = 0; /** \brief Transfer the data from the socket */ -- cgit v1.2.3 From d94b1d80d8326334d17f6a43061368e783b8e0aa Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 11 Aug 2016 18:24:35 +0200 Subject: don't sent Range requests if we know its not accepted If the server told us in a previous request that it isn't supporting Ranges with bytes via an Accept-Ranges header missing bytes, we don't try to formulate requests using Ranges. --- methods/http.cc | 2 +- methods/https.cc | 15 ++++++++------- methods/server.cc | 10 ++++++++++ methods/server.h | 1 + 4 files changed, 20 insertions(+), 8 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 8d34aa6e3..11136bb9a 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -906,7 +906,7 @@ void HttpMethod::SendReq(FetchItem *Itm) // Check for a partial file and send if-queries accordingly struct stat SBuf; - if (stat(Itm->DestFile.c_str(),&SBuf) >= 0 && SBuf.st_size > 0) + if (Server->RangesAllowed && stat(Itm->DestFile.c_str(),&SBuf) >= 0 && SBuf.st_size > 0) Req << "Range: bytes=" << std::to_string(SBuf.st_size) << "-\r\n" << "If-Range: " << TimeRFC1123(SBuf.st_mtime, false) << "\r\n"; else if (Itm->LastModified != 0) diff --git a/methods/https.cc b/methods/https.cc index c86f9407e..b2d05136c 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -382,8 +382,15 @@ bool HttpsMethod::Fetch(FetchItem *Itm) headers = curl_slist_append(headers, "Accept: text/*"); } + // go for it - if the file exists, append on it + File = new FileFd(Itm->DestFile, FileFd::WriteAny); + if (Server == nullptr || Server->Comp(Itm->Uri) == false) + Server = CreateServerState(Itm->Uri); + else + Server->Reset(false); + // if we have the file send an if-range query with a range header - if (stat(Itm->DestFile.c_str(),&SBuf) >= 0 && SBuf.st_size > 0) + if (Server->RangesAllowed && stat(Itm->DestFile.c_str(),&SBuf) >= 0 && SBuf.st_size > 0) { std::string Buf; strprintf(Buf, "Range: bytes=%lli-", (long long) SBuf.st_size); @@ -397,12 +404,6 @@ bool HttpsMethod::Fetch(FetchItem *Itm) curl_easy_setopt(curl, CURLOPT_TIMEVALUE, Itm->LastModified); } - // go for it - if the file exists, append on it - File = new FileFd(Itm->DestFile, FileFd::WriteAny); - if (Server == nullptr || Server->Comp(Itm->Uri) == false) - Server = CreateServerState(Itm->Uri); - else - Server->Reset(false); if (Server->InitHashes(Itm->ExpectedHashes) == false) return false; diff --git a/methods/server.cc b/methods/server.cc index d0b6ef3d7..aa1ee4754 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -227,6 +227,15 @@ bool ServerState::HeaderLine(string Line) return true; } + if (stringcasecmp(Tag, "Accept-Ranges:") == 0) + { + std::string ranges = ',' + Val + ','; + ranges.erase(std::remove(ranges.begin(), ranges.end(), ' '), ranges.end()); + if (ranges.find(",bytes,") == std::string::npos) + RangesAllowed = false; + return true; + } + return true; } /*}}}*/ @@ -252,6 +261,7 @@ void ServerState::Reset(bool const Everything) /*{{{*/ if (Everything) { Persistent = false; Pipeline = false; PipelineAllowed = true; + RangesAllowed = true; } } /*}}}*/ diff --git a/methods/server.h b/methods/server.h index 63acceae6..c3adba87a 100644 --- a/methods/server.h +++ b/methods/server.h @@ -51,6 +51,7 @@ struct ServerState enum {Header, Data} State; bool Persistent; bool PipelineAllowed; + bool RangesAllowed; std::string Location; // This is a Persistent attribute of the server itself. -- cgit v1.2.3 From 9714d522056e5256f5a2de587d88eba7cb3291c2 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 12 Aug 2016 11:05:58 +0200 Subject: don't try pipelining if server closes connections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a server closes a connection after sending us a file that tends to mean that its a type of server who always closes the connection – it is therefore relatively pointless to try pipelining with it even if it isn't a problem by itself: apt is just restarting the pipeline each time after it got served one file and the connection is closed. The problem starts if one or more proxies are between the server and apt and they disagree about how the connection should be as in the bugreporters case where the responses apt gets contain both Keep-Alive and Proxy-Connection headers (which apt both ignores) indicating a proxy is trying to keep a connection open while the response also contains "Connection: close" indicating the opposite which apt understands and respects as it is required to do. We avoid stepping into this abyss by not performing pipelining anymore if we got a respond with the indication to close connection if the response was otherwise a success – error messages are sent by some servers via this method as their pages tend to be created dynamically and hence their size isn't known a priori to them. Closes: #832113 --- methods/server.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'methods') diff --git a/methods/server.cc b/methods/server.cc index aa1ee4754..af7276bad 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -208,8 +208,16 @@ bool ServerState::HeaderLine(string Line) if (stringcasecmp(Tag,"Connection:") == 0) { if (stringcasecmp(Val,"close") == 0) + { Persistent = false; - if (stringcasecmp(Val,"keep-alive") == 0) + Pipeline = false; + /* Some servers send error pages (as they are dynamically generated) + for simplicity via a connection close instead of e.g. chunked, + so assuming an always closing server only if we get a file + close */ + if (Result >= 200 && Result < 300) + PipelineAllowed = false; + } + else if (stringcasecmp(Val,"keep-alive") == 0) Persistent = true; return true; } -- cgit v1.2.3 From d1bdb73a96d01896ec8e213a0f14abc38d19a929 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 17 Aug 2016 21:53:05 +0200 Subject: methods: read config in most to least specific order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The implementation of the generic config fallback did the fallback in the wrong order so that the least specific option wasn't the last value picked but in fact the first one… doh! So in the bugreports case http -> https -> http:: -> https:: while it should have been the reverse as before. Regression-In: 30060442025824c491f58887ca7369f3c572fa57 Closes: 834642 --- methods/aptmethod.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'methods') diff --git a/methods/aptmethod.h b/methods/aptmethod.h index 38f451708..04c4fa99b 100644 --- a/methods/aptmethod.h +++ b/methods/aptmethod.h @@ -86,10 +86,10 @@ public: } std::string ConfigFind(char const * const postfix, std::string const &defValue) const APT_NONNULL(2) { - for (auto && name: methodNames) + for (auto name = methodNames.rbegin(); name != methodNames.rend(); ++name) { std::string conf; - strprintf(conf, "Acquire::%s::%s", name.c_str(), postfix); + strprintf(conf, "Acquire::%s::%s", name->c_str(), postfix); auto const value = _config->Find(conf); if (value.empty() == false) return value; -- cgit v1.2.3