From 9dd940ed722e8235c615e79b7eb688eb427e9a23 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 5 Nov 2014 18:33:07 +0100 Subject: Assert statement calls a function which may have desired side effects: 'pos_is_okay' It does not have any desired sideeffect, so we just mark it as const to properly advertise this fact to developer, compiler and linter alike. Reported-By: cppcheck Git-Dch: Ignore --- methods/rred.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'methods') diff --git a/methods/rred.cc b/methods/rred.cc index cabb3c456..774b58a40 100644 --- a/methods/rred.cc +++ b/methods/rred.cc @@ -150,11 +150,11 @@ class FileChanges { std::list::iterator where; size_t pos; // line number is as far left of iterator as possible - bool pos_is_okay(void) + bool pos_is_okay(void) const { #ifdef POSDEBUG size_t cpos = 0; - std::list::iterator x; + std::list::const_iterator x; for (x = changes.begin(); x != where; ++x) { assert(x != changes.end()); cpos += x->offset + x->add_cnt; -- cgit v1.2.3 From bf6ac7ca615922c23d1f3cf1963efc5be9c23e32 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 9 Nov 2014 15:57:43 +0100 Subject: use getline() instead of rolling our own We use it in other places already as well even though it is farly new addition to the POSIX family with 2008, but rolling our own here is really something which should be avoided in such a important method. Git-Dch: Ignore --- methods/gpgv.cc | 31 +++++-------------------------- 1 file changed, 5 insertions(+), 26 deletions(-) (limited to 'methods') diff --git a/methods/gpgv.cc b/methods/gpgv.cc index 488c16826..41f138be6 100644 --- a/methods/gpgv.cc +++ b/methods/gpgv.cc @@ -86,33 +86,12 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, FILE *pipein = fdopen(fd[0], "r"); // Loop over the output of apt-key (which really is gnupg), and check the signatures. - size_t buffersize = 64; - char *buffer = (char *) malloc(buffersize); - size_t bufferoff = 0; + size_t buffersize = 0; + char *buffer = NULL; while (1) { - int c; - - // Read a line. Sigh. - while ((c = getc(pipein)) != EOF && c != '\n') - { - if (bufferoff == buffersize) - { - char* newBuffer = (char *) realloc(buffer, buffersize *= 2); - if (newBuffer == NULL) - { - free(buffer); - return "Couldn't allocate a buffer big enough for reading"; - } - buffer = newBuffer; - } - *(buffer+bufferoff) = c; - bufferoff++; - } - if (bufferoff == 0 && c == EOF) - break; - *(buffer+bufferoff) = '\0'; - bufferoff = 0; + if (getline(&buffer, &buffersize, pipein) == -1) + break; if (Debug == true) std::clog << "Read: " << buffer << std::endl; @@ -126,7 +105,7 @@ string GPGVMethod::VerifyGetSigners(const char *file, const char *outfile, std::clog << "Got BADSIG! " << std::endl; BadSigners.push_back(string(buffer+sizeof(GNUPGPREFIX))); } - + if (strncmp(buffer, GNUPGNOPUBKEY, sizeof(GNUPGNOPUBKEY)-1) == 0) { if (Debug == true) -- cgit v1.2.3 From ed793a19ec00b83254029509bc516e3ba911c75a Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 29 Nov 2014 17:59:52 +0100 Subject: dispose http(s) 416 error page as non-content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real webservers (like apache) actually send an error page with a 416 response, but our client didn't expect it leaving the page on the socket to be parsed as response for the next request (http) or as file content (https), which isn't what we want at all… Symptom is a "Bad header line" as html usually doesn't parse that well to an http-header. This manifests itself e.g. if we have a complete file (or larger) in partial/ which isn't discarded by If-Range as the server doesn't support it (or it is just newer, think: mirror rotation). It is a sort-of regression of 78c72d0ce22e00b194251445aae306df357d5c1a, which removed the filesize - 1 trick, but this had its own problems… To properly test this our webserver gains the ability to reply with transfer-encoding: chunked as most real webservers will use it to send the dynamically generated error pages. Closes: 768797 --- methods/http.cc | 2 ++ methods/https.cc | 15 ++++++++++++--- methods/server.cc | 26 +++++++++++++++----------- methods/server.h | 5 +++-- 4 files changed, 32 insertions(+), 16 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index a5de13511..ad1347d36 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -444,6 +444,8 @@ bool HttpServerState::RunData(FileFd * const File) loss of the connection means we are done */ if (Encoding == Closes) In.Limit(-1); + else if (JunkSize != 0) + In.Limit(JunkSize); else In.Limit(Size - StartPos); diff --git a/methods/https.cc b/methods/https.cc index 366148e19..23b3a10d4 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -69,6 +69,9 @@ HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) { me->Server->Result = 200; me->Server->StartPos = me->Server->Size; + // the actual size is not important for https as curl will deal with it + // by itself and e.g. doesn't bother us with transport-encoding… + me->Server->JunkSize = std::numeric_limits::max(); } else me->Server->StartPos = 0; @@ -86,19 +89,25 @@ size_t HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) { HttpsMethod *me = (HttpsMethod *)userp; + size_t buffer_size = size * nmemb; + // we don't need to count the junk here, just drop anything we get as + // we don't always know how long it would be, e.g. in chunked encoding. + if (me->Server->JunkSize != 0) + return buffer_size; if (me->Res.Size == 0) me->URIStart(me->Res); - if(me->File->Write(buffer, size*nmemb) != true) + if(me->File->Write(buffer, buffer_size) != true) return false; if(me->Queue->MaximumSize > 0 && me->File->Tell() > me->Queue->MaximumSize) { me->SetFailReason("MaximumSizeExceeded"); - return _error->Error("Writing more data than expected (%llu > %llu)", + _error->Error("Writing more data than expected (%llu > %llu)", me->TotalWritten, me->Queue->MaximumSize); + return 0; } - return size*nmemb; + return buffer_size; } int diff --git a/methods/server.cc b/methods/server.cc index c4689ff12..9b3d39cf2 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -55,6 +55,7 @@ ServerState::RunHeadersResult ServerState::RunHeaders(FileFd * const File, Minor = 0; Result = 0; Size = 0; + JunkSize = 0; StartPos = 0; Encoding = Closes; HaveContent = false; @@ -163,14 +164,14 @@ bool ServerState::HeaderLine(string Line) Encoding = Stream; HaveContent = true; - // The length is already set from the Content-Range header - if (StartPos != 0) - return true; + unsigned long long * SizePtr = &Size; + if (Result == 416) + SizePtr = &JunkSize; - Size = strtoull(Val.c_str(), NULL, 10); - if (Size >= std::numeric_limits::max()) + *SizePtr = strtoull(Val.c_str(), NULL, 10); + if (*SizePtr >= std::numeric_limits::max()) return _error->Errno("HeaderLine", _("The HTTP server sent an invalid Content-Length header")); - else if (Size == 0) + else if (*SizePtr == 0) HaveContent = false; return true; } @@ -187,10 +188,7 @@ bool ServerState::HeaderLine(string Line) // §14.16 says 'byte-range-resp-spec' should be a '*' in case of 416 if (Result == 416 && sscanf(Val.c_str(), "bytes */%llu",&Size) == 1) - { - StartPos = 1; // ignore Content-Length, it would override Size - HaveContent = false; - } + ; // we got the expected filesize which is all we wanted else if (sscanf(Val.c_str(),"bytes %llu-%*u/%llu",&StartPos,&Size) != 2) return _error->Error(_("The HTTP server sent an invalid Content-Range header")); if ((unsigned long long)StartPos > Size) @@ -308,9 +306,15 @@ ServerMethod::DealWithHeaders(FetchResult &Res) if ((unsigned long long)SBuf.st_size == Server->Size) { // the file is completely downloaded, but was not moved + if (Server->HaveContent == true) + { + // Send to error page to dev/null + FileFd DevNull("/dev/null",FileFd::WriteExists); + Server->RunData(&DevNull); + } + Server->HaveContent = false; Server->StartPos = Server->Size; Server->Result = 200; - Server->HaveContent = false; } else if (unlink(Queue->DestFile.c_str()) == 0) { diff --git a/methods/server.h b/methods/server.h index 7d5198478..b974ec89a 100644 --- a/methods/server.h +++ b/methods/server.h @@ -34,7 +34,8 @@ struct ServerState char Code[360]; // These are some statistics from the last parsed header lines - unsigned long long Size; + unsigned long long Size; // size of the usable content (aka: the file) + unsigned long long JunkSize; // size of junk content (aka: server error pages) unsigned long long StartPos; time_t Date; bool HaveContent; @@ -73,7 +74,7 @@ struct ServerState RunHeadersResult RunHeaders(FileFd * const File, const std::string &Uri); 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'; Size = 0; + virtual void Reset() {Major = 0; Minor = 0; Result = 0; Code[0] = '\0'; Size = 0; JunkSize = 0; StartPos = 0; Encoding = Closes; time(&Date); HaveContent = false; State = Header; Persistent = false; Pipeline = true; MaximumSize = 0;}; virtual bool WriteResponse(std::string const &Data) = 0; -- cgit v1.2.3 From 92e8c1ff287ab829de825e00cdf94744e699ff97 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 29 Nov 2014 17:59:52 +0100 Subject: dispose http(s) 416 error page as non-content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Real webservers (like apache) actually send an error page with a 416 response, but our client didn't expect it leaving the page on the socket to be parsed as response for the next request (http) or as file content (https), which isn't what we want at all… Symptom is a "Bad header line" as html usually doesn't parse that well to an http-header. This manifests itself e.g. if we have a complete file (or larger) in partial/ which isn't discarded by If-Range as the server doesn't support it (or it is just newer, think: mirror rotation). It is a sort-of regression of 78c72d0ce22e00b194251445aae306df357d5c1a, which removed the filesize - 1 trick, but this had its own problems… To properly test this our webserver gains the ability to reply with transfer-encoding: chunked as most real webservers will use it to send the dynamically generated error pages. (The tests and their binary helpers had to be slightly modified to apply, but the patch to fix the issue itself is unchanged.) Closes: 768797 --- methods/http.cc | 2 ++ methods/https.cc | 12 ++++++++++-- methods/server.cc | 26 +++++++++++++++----------- methods/server.h | 5 +++-- 4 files changed, 30 insertions(+), 15 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index f2a4a4db6..1b996db98 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -440,6 +440,8 @@ bool HttpServerState::RunData(FileFd * const File) loss of the connection means we are done */ if (Encoding == Closes) In.Limit(-1); + else if (JunkSize != 0) + In.Limit(JunkSize); else In.Limit(Size - StartPos); diff --git a/methods/https.cc b/methods/https.cc index 0499af0c5..65a744e2a 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -59,6 +59,9 @@ HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) { me->Server->Result = 200; me->Server->StartPos = me->Server->Size; + // the actual size is not important for https as curl will deal with it + // by itself and e.g. doesn't bother us with transport-encoding… + me->Server->JunkSize = std::numeric_limits::max(); } else me->Server->StartPos = 0; @@ -76,13 +79,18 @@ size_t HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) { HttpsMethod *me = (HttpsMethod *)userp; + size_t buffer_size = size * nmemb; + // we don't need to count the junk here, just drop anything we get as + // we don't always know how long it would be, e.g. in chunked encoding. + if (me->Server->JunkSize != 0) + return buffer_size; if (me->Res.Size == 0) me->URIStart(me->Res); - if(me->File->Write(buffer, size*nmemb) != true) + if(me->File->Write(buffer, buffer_size) != true) return false; - return size*nmemb; + return buffer_size; } int diff --git a/methods/server.cc b/methods/server.cc index 92d94e638..cb0341d5f 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -55,6 +55,7 @@ ServerState::RunHeadersResult ServerState::RunHeaders(FileFd * const File, Minor = 0; Result = 0; Size = 0; + JunkSize = 0; StartPos = 0; Encoding = Closes; HaveContent = false; @@ -163,14 +164,14 @@ bool ServerState::HeaderLine(string Line) Encoding = Stream; HaveContent = true; - // The length is already set from the Content-Range header - if (StartPos != 0) - return true; + unsigned long long * SizePtr = &Size; + if (Result == 416) + SizePtr = &JunkSize; - Size = strtoull(Val.c_str(), NULL, 10); - if (Size >= std::numeric_limits::max()) + *SizePtr = strtoull(Val.c_str(), NULL, 10); + if (*SizePtr >= std::numeric_limits::max()) return _error->Errno("HeaderLine", _("The HTTP server sent an invalid Content-Length header")); - else if (Size == 0) + else if (*SizePtr == 0) HaveContent = false; return true; } @@ -187,10 +188,7 @@ bool ServerState::HeaderLine(string Line) // §14.16 says 'byte-range-resp-spec' should be a '*' in case of 416 if (Result == 416 && sscanf(Val.c_str(), "bytes */%llu",&Size) == 1) - { - StartPos = 1; // ignore Content-Length, it would override Size - HaveContent = false; - } + ; // we got the expected filesize which is all we wanted else if (sscanf(Val.c_str(),"bytes %llu-%*u/%llu",&StartPos,&Size) != 2) return _error->Error(_("The HTTP server sent an invalid Content-Range header")); if ((unsigned long long)StartPos > Size) @@ -308,9 +306,15 @@ ServerMethod::DealWithHeaders(FetchResult &Res) if ((unsigned long long)SBuf.st_size == Server->Size) { // the file is completely downloaded, but was not moved + if (Server->HaveContent == true) + { + // Send to error page to dev/null + FileFd DevNull("/dev/null",FileFd::WriteExists); + Server->RunData(&DevNull); + } + Server->HaveContent = false; Server->StartPos = Server->Size; Server->Result = 200; - Server->HaveContent = false; } else if (unlink(Queue->DestFile.c_str()) == 0) { diff --git a/methods/server.h b/methods/server.h index f5e68d902..1b81e3549 100644 --- a/methods/server.h +++ b/methods/server.h @@ -34,7 +34,8 @@ struct ServerState char Code[360]; // These are some statistics from the last parsed header lines - unsigned long long Size; + unsigned long long Size; // size of the usable content (aka: the file) + unsigned long long JunkSize; // size of junk content (aka: server error pages) unsigned long long StartPos; time_t Date; bool HaveContent; @@ -71,7 +72,7 @@ struct ServerState RunHeadersResult RunHeaders(FileFd * const File, const std::string &Uri); 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'; Size = 0; + virtual void Reset() {Major = 0; Minor = 0; Result = 0; Code[0] = '\0'; Size = 0; JunkSize = 0; StartPos = 0; Encoding = Closes; time(&Date); HaveContent = false; State = Header; Persistent = false; Pipeline = true;}; virtual bool WriteResponse(std::string const &Data) = 0; -- cgit v1.2.3 From 9127d7aecf01f2999a2589e4b0503288518b2927 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 5 Jan 2015 10:27:53 +0100 Subject: Fix missing URIStart() for https downloads Add a explicit ReceivedData to HttpsMethod that indicates when we got data from the connection so that we can send URISTart() to the parent. This is needed because URIStart got moved in f9b4f12d from the progress_callback to write_data() and it only checks for Res.Size. In the old code if progress_callback is called by libcurl (and sets Res.Size) before write_data is called then URIStart() is never send. Making this a explicit ReceivedData variable fixes this issue. --- methods/https.cc | 9 +++++++-- methods/https.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index 65a744e2a..3a5981b58 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -85,8 +85,12 @@ HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) if (me->Server->JunkSize != 0) return buffer_size; - if (me->Res.Size == 0) + if (me->ReceivedData == false) + { me->URIStart(me->Res); + me->ReceivedData = true; + } + if(me->File->Write(buffer, buffer_size) != true) return false; @@ -95,7 +99,7 @@ HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) int HttpsMethod::progress_callback(void *clientp, double dltotal, double /*dlnow*/, - double /*ultotal*/, double /*ulnow*/) + double /*ultotal*/, double /*ulnow*/) { HttpsMethod *me = (HttpsMethod *)clientp; if(dltotal > 0 && me->Res.Size == 0) { @@ -179,6 +183,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm) char curl_errorstr[CURL_ERROR_SIZE]; URI Uri = Itm->Uri; string remotehost = Uri.Host; + ReceivedData = false; // TODO: // - http::Pipeline-Depth diff --git a/methods/https.h b/methods/https.h index faac8a3cd..411b71440 100644 --- a/methods/https.h +++ b/methods/https.h @@ -66,6 +66,7 @@ class HttpsMethod : public pkgAcqMethod CURL *curl; FetchResult Res; HttpsServerState *Server; + bool ReceivedData; public: FileFd *File; -- cgit v1.2.3 From 0c2dc43d4fe1d026650b5e2920a021557f9534a6 Mon Sep 17 00:00:00 2001 From: Tomasz Buchert Date: Mon, 16 Feb 2015 00:57:29 +0100 Subject: Fix crash in the apt-transport-https when Owner is NULL Do not crash in ServerState::HeaderLine if there is no Owner. Closes: #778375 --- methods/server.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'methods') diff --git a/methods/server.cc b/methods/server.cc index cb0341d5f..e321e0230 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -129,7 +129,7 @@ bool ServerState::HeaderLine(string Line) if (elements == 3) { Code[0] = '\0'; - if (Owner->Debug == true) + if (Owner != NULL && Owner->Debug == true) clog << "HTTP server doesn't give Reason-Phrase for " << Result << std::endl; } else if (elements != 4) -- cgit v1.2.3 From 905fba60a046646a26a56b4c5d4a5dc7d5906f0d Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 9 Mar 2015 01:54:46 +0100 Subject: derive more of https from http method Bug #778375 uncovered that https wasn't properly integrated in the class family tree of http as it was supposed to be leading to a NULL pointer dereference. Fixing this 'properly' was deemed to much diff for practically no gain that late in the release, so commit 0c2dc43d4fe1d026650b5e2920a021557f9534a6 just fixed the synptom, while this commit here is fixing the cause plus adding a test. --- methods/http.cc | 2 -- methods/https.cc | 37 ++++++++++++++++++++++--------------- methods/https.h | 16 ++++++++++------ methods/server.cc | 7 ++++++- methods/server.h | 3 ++- 5 files changed, 40 insertions(+), 25 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index ad1347d36..021b284d0 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -772,8 +772,6 @@ bool HttpMethod::Configuration(string Message) if (ServerMethod::Configuration(Message) == false) return false; - DropPrivsOrDie(); - AllowRedirect = _config->FindB("Acquire::http::AllowRedirect",true); PipelineDepth = _config->FindI("Acquire::http::Pipeline-Depth", PipelineDepth); diff --git a/methods/https.cc b/methods/https.cc index 37a8ff5fd..32de42e4b 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -37,16 +37,6 @@ /*}}}*/ using namespace std; -bool HttpsMethod::Configuration(std::string Message) -{ - if (pkgAcqMethod::Configuration(Message) == false) - return false; - - DropPrivsOrDie(); - - return true; -} - size_t HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) { @@ -131,7 +121,7 @@ HttpsMethod::progress_callback(void *clientp, double dltotal, double /*dlnow*/, } // HttpsServerState::HttpsServerState - Constructor /*{{{*/ -HttpsServerState::HttpsServerState(URI Srv,HttpsMethod * /*Owner*/) : ServerState(Srv, NULL) +HttpsServerState::HttpsServerState(URI Srv,HttpsMethod * Owner) : ServerState(Srv, Owner) { TimeOut = _config->FindI("Acquire::https::Timeout",TimeOut); ReceivedData = false; @@ -335,13 +325,11 @@ bool HttpsMethod::Fetch(FetchItem *Itm) curl_easy_setopt(curl, CURLOPT_LOW_SPEED_TIME, timeout); // set redirect options and default to 10 redirects - bool const AllowRedirect = _config->FindB("Acquire::https::AllowRedirect", - _config->FindB("Acquire::http::AllowRedirect",true)); curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, AllowRedirect); curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 10); // debug - if(_config->FindB("Debug::Acquire::https", false)) + if (Debug == true) curl_easy_setopt(curl, CURLOPT_VERBOSE, true); // error handling @@ -378,7 +366,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm) // go for it - if the file exists, append on it File = new FileFd(Itm->DestFile, FileFd::WriteAny); - Server = new HttpsServerState(Itm->Uri, this); + Server = CreateServerState(Itm->Uri); // keep apt updated Res.Filename = Itm->DestFile; @@ -473,6 +461,25 @@ bool HttpsMethod::Fetch(FetchItem *Itm) return true; } + /*}}}*/ +// HttpsMethod::Configuration - Handle a configuration message /*{{{*/ +bool HttpsMethod::Configuration(string Message) +{ + if (ServerMethod::Configuration(Message) == false) + return false; + + AllowRedirect = _config->FindB("Acquire::https::AllowRedirect", + _config->FindB("Acquire::http::AllowRedirect", true)); + Debug = _config->FindB("Debug::Acquire::https",false); + + return true; +} + /*}}}*/ +ServerState * HttpsMethod::CreateServerState(URI uri) /*{{{*/ +{ + return new HttpsServerState(uri, this); +} + /*}}}*/ int main() { diff --git a/methods/https.h b/methods/https.h index 6917a6ff6..433a84680 100644 --- a/methods/https.h +++ b/methods/https.h @@ -50,17 +50,14 @@ class HttpsServerState : public ServerState HttpsServerState(URI Srv, HttpsMethod *Owner); virtual ~HttpsServerState() {Close();}; - - bool ReceivedData; }; -class HttpsMethod : public pkgAcqMethod +class HttpsMethod : public ServerMethod { // minimum speed in bytes/se that triggers download timeout handling static const int DL_MIN_SPEED = 10; virtual bool Fetch(FetchItem *); - virtual bool Configuration(std::string Message); static size_t parse_header(void *buffer, size_t size, size_t nmemb, void *userp); static size_t write_data(void *buffer, size_t size, size_t nmemb, void *userp); @@ -69,12 +66,19 @@ class HttpsMethod : public pkgAcqMethod void SetupProxy(); CURL *curl; FetchResult Res; - HttpsServerState *Server; + ServerState *Server; + + // Used by ServerMethods unused by https + virtual void SendReq(FetchItem *) { exit(42); } + virtual void RotateDNS() { exit(42); } public: FileFd *File; - HttpsMethod() : pkgAcqMethod("1.2",Pipeline | SendConfig), Server(NULL), File(NULL) + virtual bool Configuration(std::string Message); + virtual ServerState * CreateServerState(URI uri); + + HttpsMethod() : ServerMethod("1.2",Pipeline | SendConfig), File(NULL) { curl = curl_easy_init(); }; diff --git a/methods/server.cc b/methods/server.cc index c17f27f73..91ec824d1 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -238,7 +238,12 @@ ServerState::ServerState(URI Srv, ServerMethod *Owner) : ServerName(Srv), TimeOu bool ServerMethod::Configuration(string Message) /*{{{*/ { - return pkgAcqMethod::Configuration(Message); + if (pkgAcqMethod::Configuration(Message) == false) + return false; + + DropPrivsOrDie(); + + return true; } /*}}}*/ diff --git a/methods/server.h b/methods/server.h index b974ec89a..3b232dcac 100644 --- a/methods/server.h +++ b/methods/server.h @@ -37,6 +37,7 @@ struct ServerState unsigned long long Size; // size of the usable content (aka: the file) unsigned long long JunkSize; // size of junk content (aka: server error pages) unsigned long long StartPos; + bool ReceivedData; time_t Date; bool HaveContent; enum {Chunked,Stream,Closes} Encoding; @@ -75,7 +76,7 @@ struct ServerState 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'; Size = 0; JunkSize = 0; - StartPos = 0; Encoding = Closes; time(&Date); HaveContent = false; + StartPos = 0; ReceivedData = false; Encoding = Closes; time(&Date); HaveContent = false; State = Header; Persistent = false; Pipeline = true; MaximumSize = 0;}; virtual bool WriteResponse(std::string const &Data) = 0; -- cgit v1.2.3 From 1296bc7c466181a7978c313c40a041b34ce3eaeb Mon Sep 17 00:00:00 2001 From: Robert Edmonds Date: Sun, 22 Mar 2015 00:12:45 -0400 Subject: HttpsMethod::Fetch(): Zero the FetchResult object when leaving due to 404 --- methods/https.cc | 2 ++ 1 file changed, 2 insertions(+) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index 3a5981b58..f2b00dd64 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -402,6 +402,8 @@ bool HttpsMethod::Fetch(FetchItem *Itm) _error->Error("%s", err); // unlink, no need keep 401/404 page content in partial/ unlink(File->Name().c_str()); + Res.Size = 0; + Res.LastModified = 0; return false; } -- cgit v1.2.3 From a2679f55b5b14092db88fab3799f06e6b68e439e Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 7 Apr 2015 14:34:04 +0200 Subject: properly handle expected filesize in https The worker expects that the methods tell him when they start or finish downloading a file. Various information pieces are passed along in this report including the (expected) filesize. https is using a "global" struct for reporting which made it 'reuse' incorrect values in some cases like a non-existent InRelease fallbacking to Release{,.gpg} resulting in an incorrect size-mismatch warning scaring and desensitizing users as well as being subject to a race between the write_data and progress callbacks generating incorrect progress reporting and potentially the same error message. Other branches as well as the bugreports contain 'better' fixes making the struct local and other sensible changes, but are larger as a result, so in this version we opted for short diff with minimal effect above else instead. Closes: 777565, 781509 Thanks: Robert Edmonds and Anders Kaseorg for initial patchs --- methods/https.cc | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index 3a5981b58..cb11159bc 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -68,6 +68,8 @@ HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) me->File->Truncate(me->Server->StartPos); me->File->Seek(me->Server->StartPos); + + me->Res.Size = me->Server->Size; } else if (me->Server->HeaderLine(line) == false) return 0; @@ -97,17 +99,6 @@ HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) return buffer_size; } -int -HttpsMethod::progress_callback(void *clientp, double dltotal, double /*dlnow*/, - double /*ultotal*/, double /*ulnow*/) -{ - HttpsMethod *me = (HttpsMethod *)clientp; - if(dltotal > 0 && me->Res.Size == 0) { - me->Res.Size = (unsigned long long)dltotal; - } - return 0; -} - // HttpsServerState::HttpsServerState - Constructor /*{{{*/ HttpsServerState::HttpsServerState(URI Srv,HttpsMethod * /*Owner*/) : ServerState(Srv, NULL) { @@ -201,10 +192,8 @@ bool HttpsMethod::Fetch(FetchItem *Itm) curl_easy_setopt(curl, CURLOPT_WRITEHEADER, this); curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_data); curl_easy_setopt(curl, CURLOPT_WRITEDATA, this); - curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, progress_callback); - curl_easy_setopt(curl, CURLOPT_PROGRESSDATA, this); // options - curl_easy_setopt(curl, CURLOPT_NOPROGRESS, false); + curl_easy_setopt(curl, CURLOPT_NOPROGRESS, true); curl_easy_setopt(curl, CURLOPT_FILETIME, true); // only allow curl to handle https, not the other stuff it supports curl_easy_setopt(curl, CURLOPT_PROTOCOLS, CURLPROTO_HTTPS); @@ -357,6 +346,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm) // go for it - if the file exists, append on it File = new FileFd(Itm->DestFile, FileFd::WriteAny); Server = new HttpsServerState(Itm->Uri, this); + Res = FetchResult(); // keep apt updated Res.Filename = Itm->DestFile; -- cgit v1.2.3 From b8eba208daebe3e3f235983e44da9c398d6f7a57 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 10 Mar 2015 14:11:54 +0100 Subject: reimplement the last uses of sprintf Working with strings c-style is complicated and error-prune, so by converting to c++ style we gain some simplicity and avoid buffer overflows by later extensions. Git-Dch: Ignore --- methods/ftp.cc | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) (limited to 'methods') diff --git a/methods/ftp.cc b/methods/ftp.cc index 0504e5872..7764acf6a 100644 --- a/methods/ftp.cc +++ b/methods/ftp.cc @@ -259,19 +259,21 @@ bool FTPConn::Login() { if (Opts->Value.empty() == true) continue; - + // Substitute the variables into the command - char SitePort[20]; - if (ServerName.Port != 0) - sprintf(SitePort,"%u",ServerName.Port); - else - strcpy(SitePort,"21"); string Tmp = Opts->Value; Tmp = SubstVar(Tmp,"$(PROXY_USER)",Proxy.User); Tmp = SubstVar(Tmp,"$(PROXY_PASS)",Proxy.Password); Tmp = SubstVar(Tmp,"$(SITE_USER)",User); Tmp = SubstVar(Tmp,"$(SITE_PASS)",Pass); - Tmp = SubstVar(Tmp,"$(SITE_PORT)",SitePort); + if (ServerName.Port != 0) + { + std::string SitePort; + strprintf(SitePort, "%u", ServerName.Port); + Tmp = SubstVar(Tmp,"$(SITE_PORT)", SitePort); + } + else + Tmp = SubstVar(Tmp,"$(SITE_PORT)", "21"); Tmp = SubstVar(Tmp,"$(SITE)",ServerName.Host); // Send the command -- cgit v1.2.3 From 3a53f6a1510d332e24c3330a69b987f2341d1a94 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 13 Apr 2015 12:57:22 -0400 Subject: Revert "HttpsMethod::Fetch(): Zero the FetchResult object when leaving due to 404" This reverts commit 1296bc7c466181a7978c313c40a041b34ce3eaeb. --- methods/https.cc | 2 -- 1 file changed, 2 deletions(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index f2b00dd64..3a5981b58 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -402,8 +402,6 @@ bool HttpsMethod::Fetch(FetchItem *Itm) _error->Error("%s", err); // unlink, no need keep 401/404 page content in partial/ unlink(File->Name().c_str()); - Res.Size = 0; - Res.LastModified = 0; return false; } -- cgit v1.2.3 From bb948ef562862e5cc9fcfb3d7b5e41c70382adeb Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 27 Mar 2015 11:14:44 +0100 Subject: do not unlink https file on general error It might be quite interesting which file (content) made curl freak out and other methods keep the file around as well. Git-Dch: Ignore --- methods/https.cc | 1 - 1 file changed, 1 deletion(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index 32de42e4b..c69e84d3a 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -386,7 +386,6 @@ bool HttpsMethod::Fetch(FetchItem *Itm) if (success != 0) { _error->Error("%s", curl_errorstr); - unlink(File->Name().c_str()); return false; } -- cgit v1.2.3 From 27925d82dd0cbae74d48040363fe6f6c2bae5215 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 27 Mar 2015 15:53:43 +0100 Subject: improve https method queue progress reporting The worker expects that the methods tell him when they start or finish downloading a file. Various information pieces are passed along in this report including the (expected) filesize. https was using a "global" struct for reporting which made it 'reuse' incorrect values in some cases like a non-existent InRelease fallbacking to Release{,.gpg} resulting in a size-mismatch warning. Reducing the scope and redesigning the setting of the values we can fix this and related issues. Closes: 777565, 781509 Thanks: Robert Edmonds and Anders Kaseorg for initial patchs --- methods/https.cc | 82 +++++++++++++++++++++++++++----------------------------- methods/https.h | 2 +- methods/server.h | 3 +-- 3 files changed, 41 insertions(+), 46 deletions(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index c69e84d3a..70f6a1046 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -37,11 +37,17 @@ /*}}}*/ using namespace std; +struct APT_HIDDEN CURLUserPointer { + HttpsMethod * const https; + HttpsMethod::FetchResult * const Res; + CURLUserPointer(HttpsMethod * const https, HttpsMethod::FetchResult * const Res) : https(https), Res(Res) {} +}; + size_t HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) { size_t len = size * nmemb; - HttpsMethod *me = (HttpsMethod *)userp; + CURLUserPointer *me = (CURLUserPointer *)userp; std::string line((char*) buffer, len); for (--len; len > 0; --len) if (isspace(line[len]) == 0) @@ -53,23 +59,33 @@ HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) if (line.empty() == true) { - if (me->Server->Result != 416 && me->Server->StartPos != 0) + if (me->https->Server->Result != 416 && me->https->Server->StartPos != 0) ; - else if (me->Server->Result == 416 && me->Server->Size == me->File->FileSize()) + else if (me->https->Server->Result == 416 && me->https->Server->Size == me->https->File->FileSize()) { - me->Server->Result = 200; - me->Server->StartPos = me->Server->Size; + me->https->Server->Result = 200; + me->https->Server->StartPos = me->https->Server->Size; // the actual size is not important for https as curl will deal with it // by itself and e.g. doesn't bother us with transport-encoding… - me->Server->JunkSize = std::numeric_limits::max(); + me->https->Server->JunkSize = std::numeric_limits::max(); } else - me->Server->StartPos = 0; + me->https->Server->StartPos = 0; + + me->https->File->Truncate(me->https->Server->StartPos); + me->https->File->Seek(me->https->Server->StartPos); + + me->Res->LastModified = me->https->Server->Date; + me->Res->Size = me->https->Server->Size; + me->Res->ResumePoint = me->https->Server->StartPos; - me->File->Truncate(me->Server->StartPos); - me->File->Seek(me->Server->StartPos); + // we expect valid data, so tell our caller we get the file now + if (me->https->Server->Result >= 200 && me->https->Server->Result < 300 && + me->https->Server->JunkSize == 0 && + me->Res->Size != 0 && me->Res->Size > me->Res->ResumePoint) + me->https->URIStart(*me->Res); } - else if (me->Server->HeaderLine(line) == false) + else if (me->https->Server->HeaderLine(line) == false) return 0; return size*nmemb; @@ -85,12 +101,6 @@ HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) if (me->Server->JunkSize != 0) return buffer_size; - if (me->Server->ReceivedData == false) - { - me->URIStart(me->Res); - me->Server->ReceivedData = true; - } - if(me->File->Write(buffer, buffer_size) != true) return 0; @@ -109,27 +119,15 @@ HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) return buffer_size; } -int -HttpsMethod::progress_callback(void *clientp, double dltotal, double /*dlnow*/, - double /*ultotal*/, double /*ulnow*/) -{ - HttpsMethod *me = (HttpsMethod *)clientp; - if(dltotal > 0 && me->Res.Size == 0) { - me->Res.Size = (unsigned long long)dltotal; - } - return 0; -} - // HttpsServerState::HttpsServerState - Constructor /*{{{*/ HttpsServerState::HttpsServerState(URI Srv,HttpsMethod * Owner) : ServerState(Srv, Owner) { TimeOut = _config->FindI("Acquire::https::Timeout",TimeOut); - ReceivedData = false; Reset(); } /*}}}*/ -void HttpsMethod::SetupProxy() /*{{{*/ +void HttpsMethod::SetupProxy() /*{{{*/ { URI ServerName = Queue->Uri; @@ -207,16 +205,16 @@ bool HttpsMethod::Fetch(FetchItem *Itm) maybe_add_auth (Uri, _config->FindFile("Dir::Etc::netrc")); + FetchResult Res; + CURLUserPointer userp(this, &Res); // callbacks curl_easy_setopt(curl, CURLOPT_URL, static_cast(Uri).c_str()); curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, parse_header); - curl_easy_setopt(curl, CURLOPT_WRITEHEADER, this); + curl_easy_setopt(curl, CURLOPT_WRITEHEADER, &userp); curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, write_data); curl_easy_setopt(curl, CURLOPT_WRITEDATA, this); - curl_easy_setopt(curl, CURLOPT_PROGRESSFUNCTION, progress_callback); - curl_easy_setopt(curl, CURLOPT_PROGRESSDATA, this); // options - curl_easy_setopt(curl, CURLOPT_NOPROGRESS, false); + curl_easy_setopt(curl, CURLOPT_NOPROGRESS, true); curl_easy_setopt(curl, CURLOPT_FILETIME, true); // only allow curl to handle https, not the other stuff it supports curl_easy_setopt(curl, CURLOPT_PROTOCOLS, CURLPROTO_HTTPS); @@ -414,24 +412,23 @@ bool HttpsMethod::Fetch(FetchItem *Itm) return false; } - struct stat resultStat; - if (unlikely(stat(File->Name().c_str(), &resultStat) != 0)) - { - _error->Errno("stat", "Unable to access file %s", File->Name().c_str()); - return false; - } - Res.Size = resultStat.st_size; - // invalid range-request if (Server->Result == 416) { unlink(File->Name().c_str()); - Res.Size = 0; delete File; Redirect(Itm->Uri); return true; } + struct stat resultStat; + if (unlikely(stat(File->Name().c_str(), &resultStat) != 0)) + { + _error->Errno("stat", "Unable to access file %s", File->Name().c_str()); + return false; + } + Res.Size = resultStat.st_size; + // Timestamp curl_easy_getinfo(curl, CURLINFO_FILETIME, &Res.LastModified); if (Res.LastModified != -1) @@ -455,7 +452,6 @@ bool HttpsMethod::Fetch(FetchItem *Itm) URIDone(Res); // cleanup - Res.Size = 0; delete File; return true; diff --git a/methods/https.h b/methods/https.h index 433a84680..4cc48fc34 100644 --- a/methods/https.h +++ b/methods/https.h @@ -65,7 +65,6 @@ class HttpsMethod : public ServerMethod double ultotal, double ulnow); void SetupProxy(); CURL *curl; - FetchResult Res; ServerState *Server; // Used by ServerMethods unused by https @@ -77,6 +76,7 @@ class HttpsMethod : public ServerMethod virtual bool Configuration(std::string Message); virtual ServerState * CreateServerState(URI uri); + using pkgAcqMethod::FetchResult; HttpsMethod() : ServerMethod("1.2",Pipeline | SendConfig), File(NULL) { diff --git a/methods/server.h b/methods/server.h index 3b232dcac..b974ec89a 100644 --- a/methods/server.h +++ b/methods/server.h @@ -37,7 +37,6 @@ struct ServerState unsigned long long Size; // size of the usable content (aka: the file) unsigned long long JunkSize; // size of junk content (aka: server error pages) unsigned long long StartPos; - bool ReceivedData; time_t Date; bool HaveContent; enum {Chunked,Stream,Closes} Encoding; @@ -76,7 +75,7 @@ struct ServerState 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'; Size = 0; JunkSize = 0; - StartPos = 0; ReceivedData = false; Encoding = Closes; time(&Date); HaveContent = false; + StartPos = 0; Encoding = Closes; time(&Date); HaveContent = false; State = Header; Persistent = false; Pipeline = true; MaximumSize = 0;}; virtual bool WriteResponse(std::string const &Data) = 0; -- cgit v1.2.3 From 936d5613e4b6145798c5a1d0c484158115576fa8 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 27 Mar 2015 18:56:44 +0100 Subject: remove duplicated check for same file copy Git-Dch: Ignore --- methods/copy.cc | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) (limited to 'methods') diff --git a/methods/copy.cc b/methods/copy.cc index a23c0316c..f3cd84215 100644 --- a/methods/copy.cc +++ b/methods/copy.cc @@ -65,17 +65,9 @@ bool CopyMethod::Fetch(FetchItem *Itm) Res.Size = Buf.st_size; Res.Filename = Itm->DestFile; Res.LastModified = Buf.st_mtime; - Res.IMSHit = false; + Res.IMSHit = false; URIStart(Res); - // when the files are identical, just compute the hashes - if(File == Itm->DestFile) - { - CalculateHashes(Res); - URIDone(Res); - return true; - } - // just calc the hashes if the source and destination are identical if (File == Itm->DestFile) { -- cgit v1.2.3 From a09f6eb8fc67cd2d836019f448f18580396185e5 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 27 Mar 2015 19:59:44 +0100 Subject: send Alt-* info for uncompressed based on any compressions file sends information about the uncompressed file if it can find it as well as for the compressed file. This was done only for gzip so far, but we support more compression types. That this information isn't used a lot is a different story. Git-Dch: Ignore --- methods/file.cc | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) (limited to 'methods') diff --git a/methods/file.cc b/methods/file.cc index 12db62203..5d9d7b951 100644 --- a/methods/file.cc +++ b/methods/file.cc @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -33,7 +34,7 @@ class FileMethod : public pkgAcqMethod public: - FileMethod() : pkgAcqMethod("1.0",SingleInstance | LocalOnly) {}; + FileMethod() : pkgAcqMethod("1.0",SingleInstance | SendConfig | LocalOnly) {}; }; // FileMethod::Fetch - Fetch a file /*{{{*/ @@ -58,27 +59,31 @@ bool FileMethod::Fetch(FetchItem *Itm) if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) Res.IMSHit = true; } - - // See if we can compute a file without a .gz exentsion - std::string::size_type Pos = File.rfind(".gz"); - if (Pos + 3 == File.length()) + + // See if the uncompressed file exists and reuse it + std::vector extensions = APT::Configuration::getCompressorExtensions(); + for (std::vector::const_iterator ext = extensions.begin(); ext != extensions.end(); ++ext) { - File = std::string(File,0,Pos); - if (stat(File.c_str(),&Buf) == 0) + if (APT::String::Endswith(File, *ext) == true) { - FetchResult AltRes; - AltRes.Size = Buf.st_size; - AltRes.Filename = File; - AltRes.LastModified = Buf.st_mtime; - AltRes.IMSHit = false; - if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) - AltRes.IMSHit = true; - - URIDone(Res,&AltRes); - return true; - } + std::string const unfile = File.substr(0, File.length() - ext->length() - 1); + if (stat(unfile.c_str(),&Buf) == 0) + { + FetchResult AltRes; + AltRes.Size = Buf.st_size; + AltRes.Filename = unfile; + AltRes.LastModified = Buf.st_mtime; + AltRes.IMSHit = false; + if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) + AltRes.IMSHit = true; + + URIDone(Res,&AltRes); + return true; + } + // no break here as we could have situations similar to '.gz' vs '.tar.gz' here + } } - + if (Res.Filename.empty() == true) return _error->Error(_("File not found")); -- cgit v1.2.3 From 117038bac90261351518870b3f48136f134d4bfc Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 30 Mar 2015 19:52:32 +0200 Subject: handle servers closing encoded connections correctly Servers who advertise that they close the connection get the 'Closes' encoding flag, but this conflicts with servers who response with a transfer-encoding (e.g. encoding) as it is saved in the same flag. We have a better flag for the keep-alive (or not) of the connection anyway, so we check this instead of the encoding. This is in practice not much of a problem as real servers we talk to are HTTP1.1 servers (with keep-alive) and there isn't much point in doing chunked encoding if you are going to close anyway, but our simple testserver stumbles over this if pressed and its a bit cleaner, too. Git-Dch: Ignore --- methods/http.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 021b284d0..947002cc6 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -442,7 +442,7 @@ bool HttpServerState::RunData(FileFd * const File) { /* Closes encoding is used when the server did not specify a size, the loss of the connection means we are done */ - if (Encoding == Closes) + if (Persistent == false) In.Limit(-1); else if (JunkSize != 0) In.Limit(JunkSize); @@ -524,7 +524,7 @@ bool HttpServerState::Die(FileFd &File) // See if this is because the server finished the data stream if (In.IsLimit() == false && State != HttpServerState::Header && - Encoding != HttpServerState::Closes) + Persistent == true) { Close(); if (LErrno == 0) @@ -571,7 +571,7 @@ bool HttpServerState::Flush(FileFd * const File) return true; } - if (In.IsLimit() == true || Encoding == ServerState::Closes) + if (In.IsLimit() == true || Persistent == false) return true; } return false; -- cgit v1.2.3 From 9224ce3d4d1ea0428a70e75134998e08aa45b1e6 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 30 Mar 2015 20:47:13 +0200 Subject: calculate only expected hashes in methods Methods get told which hashes are expected by the acquire system, which means we can use this list to restrict what we calculate in the methods as any extra we are calculating is wasted effort as we can't compare it with anything anyway. Adding support for a new hash algorithm is therefore 'free' now and if a algorithm is no longer provided in a repository for a file, we automatically stop calculating it. In practice this results in a speed-up in Debian as we don't have SHA512 here (so far), so we practically stop calculating it. --- methods/cdrom.cc | 2 +- methods/copy.cc | 10 +++++----- methods/file.cc | 2 +- methods/ftp.cc | 2 +- methods/gzip.cc | 2 +- methods/http.cc | 14 +++++++------- methods/http.h | 2 +- methods/https.cc | 2 +- methods/https.h | 2 +- methods/rred.cc | 2 +- methods/rsh.cc | 2 +- methods/server.cc | 2 +- methods/server.h | 2 +- 13 files changed, 23 insertions(+), 23 deletions(-) (limited to 'methods') diff --git a/methods/cdrom.cc b/methods/cdrom.cc index 74e2ecc6b..10cb29f66 100644 --- a/methods/cdrom.cc +++ b/methods/cdrom.cc @@ -266,7 +266,7 @@ bool CDROMMethod::Fetch(FetchItem *Itm) Res.LastModified = Buf.st_mtime; Res.Size = Buf.st_size; - Hashes Hash; + Hashes Hash(Itm->ExpectedHashes); FileFd Fd(Res.Filename, FileFd::ReadOnly); Hash.AddFD(Fd); Res.TakeHashes(Hash); diff --git a/methods/copy.cc b/methods/copy.cc index f3cd84215..a8e289df5 100644 --- a/methods/copy.cc +++ b/methods/copy.cc @@ -28,16 +28,16 @@ class CopyMethod : public pkgAcqMethod { virtual bool Fetch(FetchItem *Itm); - void CalculateHashes(FetchResult &Res); + void CalculateHashes(FetchItem const * const Itm, FetchResult &Res); public: CopyMethod() : pkgAcqMethod("1.0",SingleInstance | SendConfig) {}; }; -void CopyMethod::CalculateHashes(FetchResult &Res) +void CopyMethod::CalculateHashes(FetchItem const * const Itm, FetchResult &Res) { - Hashes Hash; + Hashes Hash(Itm->ExpectedHashes); FileFd::CompressMode CompressMode = FileFd::None; if (_config->FindB("Acquire::GzipIndexes", false) == true) CompressMode = FileFd::Extension; @@ -71,7 +71,7 @@ bool CopyMethod::Fetch(FetchItem *Itm) // just calc the hashes if the source and destination are identical if (File == Itm->DestFile) { - CalculateHashes(Res); + CalculateHashes(Itm, Res); URIDone(Res); return true; } @@ -104,7 +104,7 @@ bool CopyMethod::Fetch(FetchItem *Itm) if (utimes(Res.Filename.c_str(), times) != 0) return _error->Errno("utimes",_("Failed to set modification time")); - CalculateHashes(Res); + CalculateHashes(Itm, Res); URIDone(Res); return true; diff --git a/methods/file.cc b/methods/file.cc index 5d9d7b951..043ab04b8 100644 --- a/methods/file.cc +++ b/methods/file.cc @@ -87,7 +87,7 @@ bool FileMethod::Fetch(FetchItem *Itm) if (Res.Filename.empty() == true) return _error->Error(_("File not found")); - Hashes Hash; + Hashes Hash(Itm->ExpectedHashes); FileFd Fd(Res.Filename, FileFd::ReadOnly); Hash.AddFD(Fd); Res.TakeHashes(Hash); diff --git a/methods/ftp.cc b/methods/ftp.cc index 7764acf6a..92d8573f1 100644 --- a/methods/ftp.cc +++ b/methods/ftp.cc @@ -1064,7 +1064,7 @@ bool FtpMethod::Fetch(FetchItem *Itm) } // Open the file - Hashes Hash; + Hashes Hash(Itm->ExpectedHashes); { FileFd Fd(Itm->DestFile,FileFd::WriteAny); if (_error->PendingError() == true) diff --git a/methods/gzip.cc b/methods/gzip.cc index 387c05f2e..65519633c 100644 --- a/methods/gzip.cc +++ b/methods/gzip.cc @@ -91,7 +91,7 @@ bool GzipMethod::Fetch(FetchItem *Itm) return false; // Read data from source, generate checksums and write - Hashes Hash; + Hashes Hash(Itm->ExpectedHashes); bool Failed = false; while (1) { diff --git a/methods/http.cc b/methods/http.cc index 947002cc6..e4773b0e2 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -64,8 +64,8 @@ const unsigned int CircleBuf::BW_HZ=10; // CircleBuf::CircleBuf - Circular input buffer /*{{{*/ // --------------------------------------------------------------------- /* */ -CircleBuf::CircleBuf(unsigned long long Size) - : Size(Size), Hash(0), TotalWriten(0) +CircleBuf::CircleBuf(unsigned long long Size) + : Size(Size), Hash(NULL), TotalWriten(0) { Buf = new unsigned char[Size]; Reset(); @@ -84,10 +84,10 @@ void CircleBuf::Reset() TotalWriten = 0; MaxGet = (unsigned long long)-1; OutQueue = string(); - if (Hash != 0) + if (Hash != NULL) { delete Hash; - Hash = new Hashes; + Hash = NULL; } } /*}}}*/ @@ -222,7 +222,7 @@ bool CircleBuf::Write(int Fd) TotalWriten += Res; - if (Hash != 0) + if (Hash != NULL) Hash->Add(Buf + (OutP%Size),Res); OutP += Res; @@ -484,10 +484,10 @@ APT_PURE bool HttpServerState::IsOpen() /*{{{*/ return (ServerFd != -1); } /*}}}*/ -bool HttpServerState::InitHashes(FileFd &File) /*{{{*/ +bool HttpServerState::InitHashes(FileFd &File, HashStringList const &ExpectedHashes)/*{{{*/ { delete In.Hash; - In.Hash = new Hashes; + In.Hash = new Hashes(ExpectedHashes); // Set the expected size and read file for the hashes File.Truncate(StartPos); diff --git a/methods/http.h b/methods/http.h index 40a88a7be..6dc872659 100644 --- a/methods/http.h +++ b/methods/http.h @@ -111,7 +111,7 @@ struct HttpServerState: public ServerState virtual bool Open(); virtual bool IsOpen(); virtual bool Close(); - virtual bool InitHashes(FileFd &File); + virtual bool InitHashes(FileFd &File, HashStringList const &ExpectedHashes); virtual Hashes * GetHashes(); virtual bool Die(FileFd &File); virtual bool Flush(FileFd * const File); diff --git a/methods/https.cc b/methods/https.cc index 70f6a1046..81903b239 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -443,7 +443,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm) Res.LastModified = resultStat.st_mtime; // take hashes - Hashes Hash; + Hashes Hash(Itm->ExpectedHashes); FileFd Fd(Res.Filename, FileFd::ReadOnly); Hash.AddFD(Fd); Res.TakeHashes(Hash); diff --git a/methods/https.h b/methods/https.h index 4cc48fc34..dc0ff3322 100644 --- a/methods/https.h +++ b/methods/https.h @@ -42,7 +42,7 @@ class HttpsServerState : public ServerState virtual bool Open() { return false; } virtual bool IsOpen() { return false; } virtual bool Close() { return false; } - virtual bool InitHashes(FileFd &/*File*/) { return false; } + virtual bool InitHashes(FileFd &/*File*/, HashStringList const &/*ExpectedHashes*/) { return false; } virtual Hashes * GetHashes() { return NULL; } virtual bool Die(FileFd &/*File*/) { return false; } virtual bool Flush(FileFd * const /*File*/) { return false; } diff --git a/methods/rred.cc b/methods/rred.cc index 774b58a40..554ac99b4 100644 --- a/methods/rred.cc +++ b/methods/rred.cc @@ -581,7 +581,7 @@ class RredMethod : public pkgAcqMethod { FILE *inp = fopen(Path.c_str(), "r"); FILE *out = fopen(Itm->DestFile.c_str(), "w"); - Hashes hash; + Hashes hash(Itm->ExpectedHashes); patch.apply_against_file(out, inp, &hash); diff --git a/methods/rsh.cc b/methods/rsh.cc index 0e949160b..52349c61c 100644 --- a/methods/rsh.cc +++ b/methods/rsh.cc @@ -477,7 +477,7 @@ bool RSHMethod::Fetch(FetchItem *Itm) } // Open the file - Hashes Hash; + Hashes Hash(Itm->ExpectedHashes); { FileFd Fd(Itm->DestFile,FileFd::WriteAny); if (_error->PendingError() == true) diff --git a/methods/server.cc b/methods/server.cc index 91ec824d1..e403f1071 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -357,7 +357,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res) FailFd = File->Fd(); FailTime = Server->Date; - if (Server->InitHashes(*File) == false) + if (Server->InitHashes(*File, Queue->ExpectedHashes) == false) { _error->Errno("read",_("Problem hashing file")); return ERROR_NOT_FROM_SERVER; diff --git a/methods/server.h b/methods/server.h index b974ec89a..45622dd34 100644 --- a/methods/server.h +++ b/methods/server.h @@ -85,7 +85,7 @@ struct ServerState virtual bool Open() = 0; virtual bool IsOpen() = 0; virtual bool Close() = 0; - virtual bool InitHashes(FileFd &File) = 0; + virtual bool InitHashes(FileFd &File, HashStringList const &ExpectedHashes) = 0; virtual Hashes * GetHashes() = 0; virtual bool Die(FileFd &File) = 0; virtual bool Flush(FileFd * const File) = 0; -- cgit v1.2.3 From 34faa8f7ae2526f46cd1f84bb6962ad06d841e5e Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 11 Apr 2015 10:23:52 +0200 Subject: calculate hashes while downloading in https MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We do this in HTTP already to give the CPU some exercise while the disk is heavily spinning (or flashing?) to store the data avoiding the need to reread the entire file again later on to calculate the hashes – which happens outside of the eyes of progress reporting, so you might ended up with a bunch of https workers 'stuck' at 100% while they were busy calculating hashes. This is a bummer for everyone using apt as a connection speedtest as the https method works slower now (not really, it just isn't reporting done too early anymore). --- methods/http.cc | 8 +++----- methods/http.h | 2 +- methods/https.cc | 38 ++++++++++++++++++++++++++------------ methods/https.h | 6 ++++-- methods/server.cc | 8 +++++++- methods/server.h | 3 ++- 6 files changed, 43 insertions(+), 22 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index e4773b0e2..af3d5ccb6 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -484,16 +484,14 @@ APT_PURE bool HttpServerState::IsOpen() /*{{{*/ return (ServerFd != -1); } /*}}}*/ -bool HttpServerState::InitHashes(FileFd &File, HashStringList const &ExpectedHashes)/*{{{*/ +bool HttpServerState::InitHashes(HashStringList const &ExpectedHashes) /*{{{*/ { delete In.Hash; In.Hash = new Hashes(ExpectedHashes); - - // Set the expected size and read file for the hashes - File.Truncate(StartPos); - return In.Hash->AddFD(File, StartPos); + return true; } /*}}}*/ + APT_PURE Hashes * HttpServerState::GetHashes() /*{{{*/ { return In.Hash; diff --git a/methods/http.h b/methods/http.h index 6dc872659..e73871931 100644 --- a/methods/http.h +++ b/methods/http.h @@ -111,7 +111,7 @@ struct HttpServerState: public ServerState virtual bool Open(); virtual bool IsOpen(); virtual bool Close(); - virtual bool InitHashes(FileFd &File, HashStringList const &ExpectedHashes); + virtual bool InitHashes(HashStringList const &ExpectedHashes); virtual Hashes * GetHashes(); virtual bool Die(FileFd &File); virtual bool Flush(FileFd * const File); diff --git a/methods/https.cc b/methods/https.cc index 81903b239..c6b75d9ad 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -72,18 +72,18 @@ HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) else me->https->Server->StartPos = 0; - me->https->File->Truncate(me->https->Server->StartPos); - me->https->File->Seek(me->https->Server->StartPos); - me->Res->LastModified = me->https->Server->Date; me->Res->Size = me->https->Server->Size; me->Res->ResumePoint = me->https->Server->StartPos; // we expect valid data, so tell our caller we get the file now - if (me->https->Server->Result >= 200 && me->https->Server->Result < 300 && - me->https->Server->JunkSize == 0 && - me->Res->Size != 0 && me->Res->Size > me->Res->ResumePoint) - me->https->URIStart(*me->Res); + if (me->https->Server->Result >= 200 && me->https->Server->Result < 300) + { + if (me->https->Server->JunkSize == 0 && me->Res->Size != 0 && me->Res->Size > me->Res->ResumePoint) + me->https->URIStart(*me->Res); + if (me->https->Server->AddPartialFileToHashes(*(me->https->File)) == false) + return 0; + } } else if (me->https->Server->HeaderLine(line) == false) return 0; @@ -116,16 +116,31 @@ HttpsMethod::write_data(void *buffer, size_t size, size_t nmemb, void *userp) } } + if (me->Server->GetHashes()->Add((unsigned char const * const)buffer, buffer_size) == false) + return 0; + return buffer_size; } // HttpsServerState::HttpsServerState - Constructor /*{{{*/ -HttpsServerState::HttpsServerState(URI Srv,HttpsMethod * Owner) : ServerState(Srv, Owner) +HttpsServerState::HttpsServerState(URI Srv,HttpsMethod * Owner) : ServerState(Srv, Owner), Hash(NULL) { TimeOut = _config->FindI("Acquire::https::Timeout",TimeOut); Reset(); } /*}}}*/ +bool HttpsServerState::InitHashes(HashStringList const &ExpectedHashes) /*{{{*/ +{ + delete Hash; + Hash = new Hashes(ExpectedHashes); + return true; +} + /*}}}*/ +APT_PURE Hashes * HttpsServerState::GetHashes() /*{{{*/ +{ + return Hash; +} + /*}}}*/ void HttpsMethod::SetupProxy() /*{{{*/ { @@ -365,6 +380,8 @@ 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->InitHashes(Itm->ExpectedHashes) == false) + return false; // keep apt updated Res.Filename = Itm->DestFile; @@ -443,10 +460,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm) Res.LastModified = resultStat.st_mtime; // take hashes - Hashes Hash(Itm->ExpectedHashes); - FileFd Fd(Res.Filename, FileFd::ReadOnly); - Hash.AddFD(Fd); - Res.TakeHashes(Hash); + Res.TakeHashes(*(Server->GetHashes())); // keep apt updated URIDone(Res); diff --git a/methods/https.h b/methods/https.h index dc0ff3322..6e32e8d3d 100644 --- a/methods/https.h +++ b/methods/https.h @@ -29,6 +29,8 @@ class FileFd; class HttpsServerState : public ServerState { + Hashes * Hash; + protected: virtual bool ReadHeaderLines(std::string &/*Data*/) { return false; } virtual bool LoadNextResponse(bool const /*ToFile*/, FileFd * const /*File*/) { return false; } @@ -42,8 +44,8 @@ class HttpsServerState : public ServerState virtual bool Open() { return false; } virtual bool IsOpen() { return false; } virtual bool Close() { return false; } - virtual bool InitHashes(FileFd &/*File*/, HashStringList const &/*ExpectedHashes*/) { return false; } - virtual Hashes * GetHashes() { return NULL; } + virtual bool InitHashes(HashStringList const &ExpectedHashes); + virtual Hashes * GetHashes(); virtual bool Die(FileFd &/*File*/) { return false; } virtual bool Flush(FileFd * const /*File*/) { return false; } virtual bool Go(bool /*ToFile*/, FileFd * const /*File*/) { return false; } diff --git a/methods/server.cc b/methods/server.cc index e403f1071..2116926b0 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -235,6 +235,12 @@ ServerState::ServerState(URI Srv, ServerMethod *Owner) : ServerName(Srv), TimeOu Reset(); } /*}}}*/ +bool ServerState::AddPartialFileToHashes(FileFd &File) /*{{{*/ +{ + File.Truncate(StartPos); + return GetHashes()->AddFD(File, StartPos); +} + /*}}}*/ bool ServerMethod::Configuration(string Message) /*{{{*/ { @@ -357,7 +363,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res) FailFd = File->Fd(); FailTime = Server->Date; - if (Server->InitHashes(*File, Queue->ExpectedHashes) == false) + if (Server->InitHashes(Queue->ExpectedHashes) == false || Server->AddPartialFileToHashes(*File) == false) { _error->Errno("read",_("Problem hashing file")); return ERROR_NOT_FROM_SERVER; diff --git a/methods/server.h b/methods/server.h index 45622dd34..1b1f754a3 100644 --- a/methods/server.h +++ b/methods/server.h @@ -72,6 +72,7 @@ struct ServerState }; /** \brief Get the headers before the data */ RunHeadersResult RunHeaders(FileFd * const File, const std::string &Uri); + 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'; Size = 0; JunkSize = 0; @@ -85,7 +86,7 @@ struct ServerState virtual bool Open() = 0; virtual bool IsOpen() = 0; virtual bool Close() = 0; - virtual bool InitHashes(FileFd &File, HashStringList const &ExpectedHashes) = 0; + virtual bool InitHashes(HashStringList const &ExpectedHashes) = 0; virtual Hashes * GetHashes() = 0; virtual bool Die(FileFd &File) = 0; virtual bool Flush(FileFd * const File) = 0; -- cgit v1.2.3 From dcbb364fc69e1108b3fea3adb12a7ba83d9af467 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 12 May 2015 00:30:16 +0200 Subject: detect 416 complete file in partial by expected hash If we have the expected hashes we can check with them if the file we have in partial we got a 416 for is the expected file. We detected this with same-size before, but not every server sends a good Content-Range header with a 416 response. --- methods/https.cc | 37 +++++++++++++++++++++++++++++-------- methods/https.h | 1 + methods/server.cc | 19 ++++++++++++++++--- 3 files changed, 46 insertions(+), 11 deletions(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index c6b75d9ad..712e9ee73 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -40,7 +40,9 @@ using namespace std; struct APT_HIDDEN CURLUserPointer { HttpsMethod * const https; HttpsMethod::FetchResult * const Res; - CURLUserPointer(HttpsMethod * const https, HttpsMethod::FetchResult * const Res) : https(https), Res(Res) {} + HttpsMethod::FetchItem const * const Itm; + CURLUserPointer(HttpsMethod * const https, HttpsMethod::FetchResult * const Res, + HttpsMethod::FetchItem const * const Itm) : https(https), Res(Res), Itm(Itm) {} }; size_t @@ -61,13 +63,32 @@ HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) { if (me->https->Server->Result != 416 && me->https->Server->StartPos != 0) ; - else if (me->https->Server->Result == 416 && me->https->Server->Size == me->https->File->FileSize()) + else if (me->https->Server->Result == 416) { - me->https->Server->Result = 200; - me->https->Server->StartPos = me->https->Server->Size; - // the actual size is not important for https as curl will deal with it - // by itself and e.g. doesn't bother us with transport-encoding… - me->https->Server->JunkSize = std::numeric_limits::max(); + bool partialHit = false; + if (me->Itm->ExpectedHashes.usable() == true) + { + Hashes resultHashes(me->Itm->ExpectedHashes); + FileFd file(me->Itm->DestFile, FileFd::ReadOnly); + me->https->Server->Size = file.FileSize(); + me->https->Server->Date = file.ModificationTime(); + resultHashes.AddFD(file); + HashStringList const hashList = resultHashes.GetHashStringList(); + partialHit = (me->Itm->ExpectedHashes == hashList); + } + else if (me->https->Server->Result == 416 && me->https->Server->Size == me->https->File->FileSize()) + partialHit = true; + + if (partialHit == true) + { + me->https->Server->Result = 200; + me->https->Server->StartPos = me->https->Server->Size; + // the actual size is not important for https as curl will deal with it + // by itself and e.g. doesn't bother us with transport-encoding… + me->https->Server->JunkSize = std::numeric_limits::max(); + } + else + me->https->Server->StartPos = 0; } else me->https->Server->StartPos = 0; @@ -221,7 +242,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm) maybe_add_auth (Uri, _config->FindFile("Dir::Etc::netrc")); FetchResult Res; - CURLUserPointer userp(this, &Res); + CURLUserPointer userp(this, &Res, Itm); // callbacks curl_easy_setopt(curl, CURLOPT_URL, static_cast(Uri).c_str()); curl_easy_setopt(curl, CURLOPT_HEADERFUNCTION, parse_header); diff --git a/methods/https.h b/methods/https.h index 6e32e8d3d..57fc292ee 100644 --- a/methods/https.h +++ b/methods/https.h @@ -79,6 +79,7 @@ class HttpsMethod : public ServerMethod virtual bool Configuration(std::string Message); virtual ServerState * CreateServerState(URI uri); using pkgAcqMethod::FetchResult; + using pkgAcqMethod::FetchItem; HttpsMethod() : ServerMethod("1.2",Pipeline | SendConfig), File(NULL) { diff --git a/methods/server.cc b/methods/server.cc index 2116926b0..bd01c3e98 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -269,7 +269,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res) Res.LastModified = Queue->LastModified; return IMS_HIT; } - + /* Redirect * * Note that it is only OK for us to treat all redirection the same @@ -314,7 +314,20 @@ ServerMethod::DealWithHeaders(FetchResult &Res) struct stat SBuf; if (stat(Queue->DestFile.c_str(),&SBuf) >= 0 && SBuf.st_size > 0) { - if ((unsigned long long)SBuf.st_size == Server->Size) + bool partialHit = false; + if (Queue->ExpectedHashes.usable() == true) + { + Hashes resultHashes(Queue->ExpectedHashes); + FileFd file(Queue->DestFile, FileFd::ReadOnly); + Server->Size = file.FileSize(); + Server->Date = file.ModificationTime(); + resultHashes.AddFD(file); + HashStringList const hashList = resultHashes.GetHashStringList(); + partialHit = (Queue->ExpectedHashes == hashList); + } + else if ((unsigned long long)SBuf.st_size == Server->Size) + partialHit = true; + if (partialHit == true) { // the file is completely downloaded, but was not moved if (Server->HaveContent == true) @@ -351,7 +364,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res) // This is some sort of 2xx 'data follows' reply Res.LastModified = Server->Date; Res.Size = Server->Size; - + // Open the file delete File; File = new FileFd(Queue->DestFile,FileFd::WriteAny); -- cgit v1.2.3 From 8eafc759544298211cd0bfaa3919afc0fadd47d1 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Wed, 13 May 2015 16:09:12 +0200 Subject: detect Releasefile IMS hits even if the server doesn't Not all servers we are talking to support If-Modified-Since and some are not even sending Last-Modified for us, so in an effort to detect such hits we run a hashsum check on the 'old' compared to the 'new' file, we got the hashes for the 'new' already for "free" from the methods anyway and hence just need to calculated the old ones. This allows us to detect hits even with unsupported servers, which in turn means we benefit from all the new hit behavior also here. --- methods/https.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index 712e9ee73..fa143439a 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -444,7 +444,7 @@ bool HttpsMethod::Fetch(FetchItem *Itm) char err[255]; snprintf(err, sizeof(err) - 1, "HttpError%i", Server->Result); SetFailReason(err); - _error->Error("%s", err); + _error->Error("%i %s", Server->Result, Server->Code); // unlink, no need keep 401/404 page content in partial/ unlink(File->Name().c_str()); return false; -- cgit v1.2.3 From ceafe8a6edc815df2923ba892894617829e9d3c2 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 22 May 2015 15:28:53 +0200 Subject: Fix endless loop in apt-get update that can cause disk fillup The apt http code parses Content-Length and Content-Range. For both requests the variable "Size" is used and the semantic for this Size is the total file size. However Content-Length is not the entire file size for partital file requests. For servers that send the Content-Range header first and then the Content-Length header this can lead to globbing of Size so that its less than the real file size. This may lead to a subsequent passing of a negative number into the CircleBuf which leads to a endless loop that writes data. Thanks to Anton Blanchard for the analysis and initial patch. LP: #1445239 --- methods/http.cc | 2 +- methods/server.cc | 20 +++++++++++++++----- methods/server.h | 3 ++- 3 files changed, 18 insertions(+), 7 deletions(-) (limited to 'methods') diff --git a/methods/http.cc b/methods/http.cc index 1b996db98..ad90c9891 100644 --- a/methods/http.cc +++ b/methods/http.cc @@ -443,7 +443,7 @@ bool HttpServerState::RunData(FileFd * const File) else if (JunkSize != 0) In.Limit(JunkSize); else - In.Limit(Size - StartPos); + In.Limit(DownloadSize); // Just transfer the whole block. do diff --git a/methods/server.cc b/methods/server.cc index e321e0230..ba0a8864b 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -164,15 +164,22 @@ bool ServerState::HeaderLine(string Line) Encoding = Stream; HaveContent = true; - unsigned long long * SizePtr = &Size; + unsigned long long * DownloadSizePtr = &DownloadSize; if (Result == 416) - SizePtr = &JunkSize; + DownloadSizePtr = &JunkSize; - *SizePtr = strtoull(Val.c_str(), NULL, 10); - if (*SizePtr >= std::numeric_limits::max()) + *DownloadSizePtr = strtoull(Val.c_str(), NULL, 10); + if (*DownloadSizePtr >= std::numeric_limits::max()) return _error->Errno("HeaderLine", _("The HTTP server sent an invalid Content-Length header")); - else if (*SizePtr == 0) + else if (*DownloadSizePtr == 0) HaveContent = false; + + // On partial content (206) the Content-Length less than the real + // size, so do not set it here but leave that to the Content-Range + // header instead + if(Result != 206 && Size == 0) + Size = DownloadSize; + return true; } @@ -193,6 +200,9 @@ bool ServerState::HeaderLine(string Line) return _error->Error(_("The HTTP server sent an invalid Content-Range header")); if ((unsigned long long)StartPos > Size) return _error->Error(_("This HTTP server has broken range support")); + + // figure out what we will download + DownloadSize = Size - StartPos; return true; } diff --git a/methods/server.h b/methods/server.h index 1b81e3549..ed3cb456a 100644 --- a/methods/server.h +++ b/methods/server.h @@ -34,7 +34,8 @@ struct ServerState char Code[360]; // These are some statistics from the last parsed header lines - unsigned long long Size; // size of the usable content (aka: the file) + unsigned long long Size; // total size of the usable content (aka: the file) + unsigned long long DownloadSize; // size we actually download (can be smaller than Size if we have partial content) unsigned long long JunkSize; // size of junk content (aka: server error pages) unsigned long long StartPos; time_t Date; -- cgit v1.2.3 From 6291f60e86718697f261519a6818e1d5ee433216 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 22 May 2015 15:40:18 +0200 Subject: Rename "Size" in ServerState to TotalFileSize The variable "Size" was misleading and caused bug #1445239. To avoid similar issues in the future, rename it to make the meaning more obvious. git-dch: ignore --- methods/https.cc | 4 ++-- methods/server.cc | 20 ++++++++++---------- methods/server.h | 14 ++++++++++---- 3 files changed, 22 insertions(+), 16 deletions(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index 3a5981b58..12fc6c70f 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -55,10 +55,10 @@ HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) { if (me->Server->Result != 416 && me->Server->StartPos != 0) ; - else if (me->Server->Result == 416 && me->Server->Size == me->File->FileSize()) + else if (me->Server->Result == 416 && me->Server->TotalFileSize == me->File->FileSize()) { me->Server->Result = 200; - me->Server->StartPos = me->Server->Size; + me->Server->StartPos = me->Server->TotalFileSize; // the actual size is not important for https as curl will deal with it // by itself and e.g. doesn't bother us with transport-encoding… me->Server->JunkSize = std::numeric_limits::max(); diff --git a/methods/server.cc b/methods/server.cc index ba0a8864b..6c05700a5 100644 --- a/methods/server.cc +++ b/methods/server.cc @@ -54,7 +54,7 @@ ServerState::RunHeadersResult ServerState::RunHeaders(FileFd * const File, Major = 0; Minor = 0; Result = 0; - Size = 0; + TotalFileSize = 0; JunkSize = 0; StartPos = 0; Encoding = Closes; @@ -177,8 +177,8 @@ bool ServerState::HeaderLine(string Line) // On partial content (206) the Content-Length less than the real // size, so do not set it here but leave that to the Content-Range // header instead - if(Result != 206 && Size == 0) - Size = DownloadSize; + if(Result != 206 && TotalFileSize == 0) + TotalFileSize = DownloadSize; return true; } @@ -194,15 +194,15 @@ bool ServerState::HeaderLine(string Line) HaveContent = true; // §14.16 says 'byte-range-resp-spec' should be a '*' in case of 416 - if (Result == 416 && sscanf(Val.c_str(), "bytes */%llu",&Size) == 1) + if (Result == 416 && sscanf(Val.c_str(), "bytes */%llu",&TotalFileSize) == 1) ; // we got the expected filesize which is all we wanted - else if (sscanf(Val.c_str(),"bytes %llu-%*u/%llu",&StartPos,&Size) != 2) + else if (sscanf(Val.c_str(),"bytes %llu-%*u/%llu",&StartPos,&TotalFileSize) != 2) return _error->Error(_("The HTTP server sent an invalid Content-Range header")); - if ((unsigned long long)StartPos > Size) + if ((unsigned long long)StartPos > TotalFileSize) return _error->Error(_("This HTTP server has broken range support")); // figure out what we will download - DownloadSize = Size - StartPos; + DownloadSize = TotalFileSize - StartPos; return true; } @@ -313,7 +313,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res) struct stat SBuf; if (stat(Queue->DestFile.c_str(),&SBuf) >= 0 && SBuf.st_size > 0) { - if ((unsigned long long)SBuf.st_size == Server->Size) + if ((unsigned long long)SBuf.st_size == Server->TotalFileSize) { // the file is completely downloaded, but was not moved if (Server->HaveContent == true) @@ -323,7 +323,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res) Server->RunData(&DevNull); } Server->HaveContent = false; - Server->StartPos = Server->Size; + Server->StartPos = Server->TotalFileSize; Server->Result = 200; } else if (unlink(Queue->DestFile.c_str()) == 0) @@ -349,7 +349,7 @@ ServerMethod::DealWithHeaders(FetchResult &Res) // This is some sort of 2xx 'data follows' reply Res.LastModified = Server->Date; - Res.Size = Server->Size; + Res.Size = Server->TotalFileSize; // Open the file delete File; diff --git a/methods/server.h b/methods/server.h index ed3cb456a..8c14282b6 100644 --- a/methods/server.h +++ b/methods/server.h @@ -34,10 +34,16 @@ struct ServerState char Code[360]; // These are some statistics from the last parsed header lines - unsigned long long Size; // total size of the usable content (aka: the file) - unsigned long long DownloadSize; // size we actually download (can be smaller than Size if we have partial content) - unsigned long long JunkSize; // size of junk content (aka: server error pages) + + // total size of the usable content (aka: the file) + unsigned long long TotalFileSize; + // size we actually download (can be smaller than Size if we have partial content) + unsigned long long DownloadSize; + // size of junk content (aka: server error pages) + unsigned long long JunkSize; + // The start of the data (for partial content) unsigned long long StartPos; + time_t Date; bool HaveContent; enum {Chunked,Stream,Closes} Encoding; @@ -73,7 +79,7 @@ struct ServerState RunHeadersResult RunHeaders(FileFd * const File, const std::string &Uri); 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'; Size = 0; JunkSize = 0; + 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 = true;}; virtual bool WriteResponse(std::string const &Data) = 0; -- cgit v1.2.3 From 65759e00eff0513c34f584b99420b72fe0e5073e Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Fri, 22 May 2015 16:27:08 +0200 Subject: Update methods/https.cc now that ServerState::Size is renamed Git-Dch: ignore --- methods/https.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'methods') diff --git a/methods/https.cc b/methods/https.cc index 81060122c..c97367323 100644 --- a/methods/https.cc +++ b/methods/https.cc @@ -69,7 +69,7 @@ HttpsMethod::parse_header(void *buffer, size_t size, size_t nmemb, void *userp) me->File->Truncate(me->Server->StartPos); me->File->Seek(me->Server->StartPos); - me->Res.Size = me->Server->Size; + me->Res.Size = me->Server->TotalFileSize; } else if (me->Server->HeaderLine(line) == false) return 0; -- cgit v1.2.3 From 448c38bdcd72b52f11ec5f326f822cf57653f81c Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 6 Jun 2015 12:28:00 +0200 Subject: rework hashsum verification in the acquire system MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Having every item having its own code to verify the file(s) it handles is an errorprune process and easy to break, especially if items move through various stages (download, uncompress, patching, …). With a giant rework we centralize (most of) the verification to have a better enforcement rate and (hopefully) less chance for bugs, but it breaks the ABI bigtime in exchange – and as we break it anyway, it is broken even harder. It shouldn't effect most frontends as they don't deal with the acquire system at all or implement their own items, but some do and will need to be patched (might be an opportunity to use apt on-board material). The theory is simple: Items implement methods to decide if hashes need to be checked (in this stage) and to return the expected hashes for this item (in this stage). The verification itself is done in worker message passing which has the benefit that a hashsum error is now a proper error for the acquire system rather than a Done() which is later revised to a Failed(). --- methods/file.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'methods') diff --git a/methods/file.cc b/methods/file.cc index 043ab04b8..353e54bd5 100644 --- a/methods/file.cc +++ b/methods/file.cc @@ -57,7 +57,11 @@ bool FileMethod::Fetch(FetchItem *Itm) Res.LastModified = Buf.st_mtime; Res.IMSHit = false; if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) - Res.IMSHit = true; + { + unsigned long long const filesize = Itm->ExpectedHashes.FileSize(); + if (filesize != 0 && filesize == Res.Size) + Res.IMSHit = true; + } } // See if the uncompressed file exists and reuse it -- cgit v1.2.3 From 3679515479136179e0d95325a6559fcc6d0af7f8 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sat, 6 Jun 2015 19:16:45 +0200 Subject: check patch hashes in rred worker instead of in the handler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rred is responsible for unpacking and reading the patch files in one go, but we currently only have hashes for the uncompressed patch files, so the handler read the entire patch file before dispatching it to the worker which would read it again – both with an implicit uncompress. Worse, while the workers operate in parallel the handler is the central orchestration unit, so having it busy with work means the workers do (potentially) nothing. This means rred is working with 'untrusted' data, which is bad. Yet, having the unpack in the handler meant that the untrusted uncompress was done as root which isn't better either. Now, we have it at least contained in a binary which we can harden a bit better. In the long run, we want hashes for the compressed patch files through to be safe. --- methods/rred.cc | 62 +++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 10 deletions(-) (limited to 'methods') diff --git a/methods/rred.cc b/methods/rred.cc index 554ac99b4..3da33c126 100644 --- a/methods/rred.cc +++ b/methods/rred.cc @@ -388,7 +388,7 @@ class Patch { public: - void read_diff(FileFd &f) + void read_diff(FileFd &f, Hashes * const h) { char buffer[BLOCK_SIZE]; bool cmdwanted = true; @@ -396,6 +396,8 @@ class Patch { Change ch(0); while(f.ReadLine(buffer, sizeof(buffer))) { + if (h != NULL) + h->Add(buffer); if (cmdwanted) { char *m, *c; size_t s, e; @@ -519,8 +521,29 @@ class RredMethod : public pkgAcqMethod { private: bool Debug; + struct PDiffFile { + std::string FileName; + HashStringList ExpectedHashes; + PDiffFile(std::string const &FileName, HashStringList const &ExpectedHashes) : + FileName(FileName), ExpectedHashes(ExpectedHashes) {} + }; + + HashStringList ReadExpectedHashesForPatch(unsigned int const patch, std::string const &Message) + { + HashStringList ExpectedHashes; + for (char const * const * type = HashString::SupportedHashes(); *type != NULL; ++type) + { + std::string tagname; + strprintf(tagname, "Patch-%d-%s-Hash", patch, *type); + std::string const hashsum = LookupTag(Message, tagname.c_str()); + if (hashsum.empty() == false) + ExpectedHashes.push_back(HashString(*type, hashsum)); + } + return ExpectedHashes; + } + protected: - virtual bool Fetch(FetchItem *Itm) { + virtual bool URIAcquire(std::string const &Message, FetchItem *Itm) { Debug = _config->FindB("Debug::pkgAcquire::RRed", false); URI Get = Itm->Uri; std::string Path = Get.Host + Get.Path; // rred:/path - no host @@ -534,11 +557,17 @@ class RredMethod : public pkgAcqMethod { } else URIStart(Res); - std::vector patchpaths; + std::vector patchfiles; Patch patch; if (FileExists(Path + ".ed") == true) - patchpaths.push_back(Path + ".ed"); + { + HashStringList const ExpectedHashes = ReadExpectedHashesForPatch(0, Message); + std::string const FileName = Path + ".ed"; + if (ExpectedHashes.usable() == false) + return _error->Error("No hashes found for uncompressed patch: %s", FileName.c_str()); + patchfiles.push_back(PDiffFile(FileName, ExpectedHashes)); + } else { _error->PushToStack(); @@ -546,18 +575,27 @@ class RredMethod : public pkgAcqMethod { _error->RevertToStack(); std::string const baseName = Path + ".ed."; + unsigned int seen_patches = 0; for (std::vector::const_iterator p = patches.begin(); p != patches.end(); ++p) + { if (p->compare(0, baseName.length(), baseName) == 0) - patchpaths.push_back(*p); + { + HashStringList const ExpectedHashes = ReadExpectedHashesForPatch(seen_patches, Message); + if (ExpectedHashes.usable() == false) + return _error->Error("No hashes found for uncompressed patch %d: %s", seen_patches, p->c_str()); + patchfiles.push_back(PDiffFile(*p, ExpectedHashes)); + ++seen_patches; + } + } } std::string patch_name; - for (std::vector::iterator I = patchpaths.begin(); - I != patchpaths.end(); + for (std::vector::iterator I = patchfiles.begin(); + I != patchfiles.end(); ++I) { - patch_name = *I; + patch_name = I->FileName; if (Debug == true) std::clog << "Patching " << Path << " with " << patch_name << std::endl; @@ -569,8 +607,12 @@ class RredMethod : public pkgAcqMethod { _error->DumpErrors(std::cerr); abort(); } - patch.read_diff(p); + Hashes patch_hash(I->ExpectedHashes); + patch.read_diff(p, &patch_hash); p.Close(); + HashStringList const hsl = patch_hash.GetHashStringList(); + if (hsl != I->ExpectedHashes) + return _error->Error("Patch %s doesn't have the expected hashsum", patch_name.c_str()); } if (Debug == true) @@ -643,7 +685,7 @@ int main(int argc, char **argv) _error->DumpErrors(std::cerr); exit(1); } - patch.read_diff(p); + patch.read_diff(p, NULL); } if (just_diff) { -- cgit v1.2.3 From 6d3e5bd8e08564c5eb12ecd869de5bd71e25f59d Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 7 Jun 2015 02:17:15 +0200 Subject: add more parsing error checking for rred The rred parser is very accepting regarding 'invalid' files. Given that we can't trust the input it might be a bit too relaxed. In any case, checking for more errors can't hurt given that we support only a very specific subset of ed commands. --- methods/rred.cc | 70 ++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 49 insertions(+), 21 deletions(-) (limited to 'methods') diff --git a/methods/rred.cc b/methods/rred.cc index 3da33c126..81ecf8553 100644 --- a/methods/rred.cc +++ b/methods/rred.cc @@ -21,6 +21,7 @@ #include #include +#include #include #include #include @@ -35,7 +36,7 @@ class MemBlock { char *start; size_t size; char *free; - struct MemBlock *next; + MemBlock *next; MemBlock(size_t size) : size(size), next(NULL) { @@ -116,7 +117,7 @@ struct Change { size_t add_len; /* bytes */ char *add; - Change(int off) + Change(size_t off) { offset = off; del_cnt = add_cnt = add_len = 0; @@ -388,30 +389,37 @@ class Patch { public: - void read_diff(FileFd &f, Hashes * const h) + bool read_diff(FileFd &f, Hashes * const h) { char buffer[BLOCK_SIZE]; bool cmdwanted = true; - Change ch(0); - while(f.ReadLine(buffer, sizeof(buffer))) - { + Change ch(std::numeric_limits::max()); + if (f.ReadLine(buffer, sizeof(buffer)) == NULL) + return _error->Error("Reading first line of patchfile %s failed", f.Name().c_str()); + do { if (h != NULL) h->Add(buffer); if (cmdwanted) { char *m, *c; size_t s, e; - s = strtol(buffer, &m, 10); - if (m == buffer) { - s = e = ch.offset + ch.add_cnt; - c = buffer; - } else if (*m == ',') { - m++; + errno = 0; + s = strtoul(buffer, &m, 10); + if (unlikely(m == buffer || s == ULONG_MAX || errno != 0)) + return _error->Error("Parsing patchfile %s failed: Expected an effected line start", f.Name().c_str()); + else if (*m == ',') { + ++m; e = strtol(m, &c, 10); + if (unlikely(m == c || e == ULONG_MAX || errno != 0)) + return _error->Error("Parsing patchfile %s failed: Expected an effected line end", f.Name().c_str()); + if (unlikely(e < s)) + return _error->Error("Parsing patchfile %s failed: Effected lines end %lu is before start %lu", f.Name().c_str(), e, s); } else { e = s; c = m; } + if (s > ch.offset) + return _error->Error("Parsing patchfile %s failed: Effected line is after previous effected line", f.Name().c_str()); switch(*c) { case 'a': cmdwanted = false; @@ -422,6 +430,8 @@ class Patch { ch.del_cnt = 0; break; case 'c': + if (unlikely(s == 0)) + return _error->Error("Parsing patchfile %s failed: Change command can't effect line zero", f.Name().c_str()); cmdwanted = false; ch.add = NULL; ch.add_cnt = 0; @@ -430,6 +440,8 @@ class Patch { ch.del_cnt = e - s + 1; break; case 'd': + if (unlikely(s == 0)) + return _error->Error("Parsing patchfile %s failed: Delete command can't effect line zero", f.Name().c_str()); ch.offset = s - 1; ch.del_cnt = e - s + 1; ch.add = NULL; @@ -437,9 +449,11 @@ class Patch { ch.add_len = 0; filechanges.add_change(ch); break; + default: + return _error->Error("Parsing patchfile %s failed: Unknown command", f.Name().c_str()); } } else { /* !cmdwanted */ - if (buffer[0] == '.' && buffer[1] == '\n') { + if (strcmp(buffer, ".\n") == 0) { cmdwanted = true; filechanges.add_change(ch); } else { @@ -465,7 +479,8 @@ class Patch { } } } - } + } while(f.ReadLine(buffer, sizeof(buffer))); + return true; } void write_diff(FILE *f) @@ -601,14 +616,14 @@ class RredMethod : public pkgAcqMethod { << std::endl; FileFd p; + Hashes patch_hash(I->ExpectedHashes); // all patches are compressed, even if the name doesn't reflect it - if (p.Open(patch_name, FileFd::ReadOnly, FileFd::Gzip) == false) { - std::cerr << "Could not open patch file " << patch_name << std::endl; + if (p.Open(patch_name, FileFd::ReadOnly, FileFd::Gzip) == false || + patch.read_diff(p, &patch_hash) == false) + { _error->DumpErrors(std::cerr); - abort(); + return false; } - Hashes patch_hash(I->ExpectedHashes); - patch.read_diff(p, &patch_hash); p.Close(); HashStringList const hsl = patch_hash.GetHashStringList(); if (hsl != I->ExpectedHashes) @@ -624,7 +639,6 @@ class RredMethod : public pkgAcqMethod { FILE *out = fopen(Itm->DestFile.c_str(), "w"); Hashes hash(Itm->ExpectedHashes); - patch.apply_against_file(out, inp, &hash); fclose(out); @@ -657,6 +671,16 @@ class RredMethod : public pkgAcqMethod { return true; } + bool Configuration(std::string Message) + { + if (pkgAcqMethod::Configuration(Message) == false) + return false; + + DropPrivsOrDie(); + + return true; + } + public: RredMethod() : pkgAcqMethod("2.0",SingleInstance | SendConfig), Debug(false) {} }; @@ -685,7 +709,11 @@ int main(int argc, char **argv) _error->DumpErrors(std::cerr); exit(1); } - patch.read_diff(p, NULL); + if (patch.read_diff(p, NULL) == false) + { + _error->DumpErrors(std::cerr); + exit(2); + } } if (just_diff) { -- cgit v1.2.3 From 4f51fd8636592a96aecf17c8bf4cfdb3ea2207cc Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 8 Jun 2015 00:06:41 +0200 Subject: support hashes for compressed pdiff files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment we only have hashes for the uncompressed pdiff files, but via the new '$HASH-Download' field in the .diff/Index hashes can be provided for the .gz compressed pdiff file, which apt will pick up now and use to verify the download. Now, we "just" need a buy in from the creators of repositories… --- methods/rred.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'methods') diff --git a/methods/rred.cc b/methods/rred.cc index 81ecf8553..12cf2b4a5 100644 --- a/methods/rred.cc +++ b/methods/rred.cc @@ -627,7 +627,7 @@ class RredMethod : public pkgAcqMethod { p.Close(); HashStringList const hsl = patch_hash.GetHashStringList(); if (hsl != I->ExpectedHashes) - return _error->Error("Patch %s doesn't have the expected hashsum", patch_name.c_str()); + return _error->Error("Hash Sum mismatch for uncompressed patch %s", patch_name.c_str()); } if (Debug == true) -- cgit v1.2.3 From c69e8370947d765dd94f142d18dc11d5a76af443 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Tue, 9 Jun 2015 15:15:33 +0200 Subject: replace ULONG_MAX with c++ style std::numeric_limits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For some reason travis seems to be unhappy about it claiming it is not defined. Well, lets not think to deeply about it… Git-Dch: Ignore --- methods/rred.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'methods') diff --git a/methods/rred.cc b/methods/rred.cc index 12cf2b4a5..54123ab9c 100644 --- a/methods/rred.cc +++ b/methods/rred.cc @@ -405,12 +405,12 @@ class Patch { size_t s, e; errno = 0; s = strtoul(buffer, &m, 10); - if (unlikely(m == buffer || s == ULONG_MAX || errno != 0)) + if (unlikely(m == buffer || s == std::numeric_limits::max() || errno != 0)) return _error->Error("Parsing patchfile %s failed: Expected an effected line start", f.Name().c_str()); else if (*m == ',') { ++m; e = strtol(m, &c, 10); - if (unlikely(m == c || e == ULONG_MAX || errno != 0)) + if (unlikely(m == c || e == std::numeric_limits::max() || errno != 0)) return _error->Error("Parsing patchfile %s failed: Expected an effected line end", f.Name().c_str()); if (unlikely(e < s)) return _error->Error("Parsing patchfile %s failed: Effected lines end %lu is before start %lu", f.Name().c_str(), e, s); -- cgit v1.2.3 From 9f697f69cf1adaced476598cfe08ab03c76c5d18 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 15 Jun 2015 12:51:22 +0200 Subject: ensure valid or remove destination file in file method 'file' isn't using the destination file per-se, but returns another name via "Filename" header. It still should deal with destination files as they could exist (pkgAcqFile e.g. creates links in that location) and are potentially bogus. --- methods/file.cc | 44 +++++++++++++++++++++++++++++++++----------- 1 file changed, 33 insertions(+), 11 deletions(-) (limited to 'methods') diff --git a/methods/file.cc b/methods/file.cc index 353e54bd5..5d5fffa67 100644 --- a/methods/file.cc +++ b/methods/file.cc @@ -48,8 +48,24 @@ bool FileMethod::Fetch(FetchItem *Itm) if (Get.Host.empty() == false) return _error->Error(_("Invalid URI, local URIS must not start with //")); - // See if the file exists struct stat Buf; + // deal with destination files which might linger around + if (lstat(Itm->DestFile.c_str(), &Buf) == 0) + { + if ((Buf.st_mode & S_IFREG) != 0) + { + if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) + { + HashStringList const hsl = Itm->ExpectedHashes; + if (Itm->ExpectedHashes.VerifyFile(File)) + Res.IMSHit = true; + } + } + } + if (Res.IMSHit != true) + unlink(Itm->DestFile.c_str()); + + // See if the file exists if (stat(File.c_str(),&Buf) == 0) { Res.Size = Buf.st_size; @@ -65,6 +81,8 @@ bool FileMethod::Fetch(FetchItem *Itm) } // See if the uncompressed file exists and reuse it + FetchResult AltRes; + AltRes.Filename.clear(); std::vector extensions = APT::Configuration::getCompressorExtensions(); for (std::vector::const_iterator ext = extensions.begin(); ext != extensions.end(); ++ext) { @@ -73,29 +91,33 @@ bool FileMethod::Fetch(FetchItem *Itm) std::string const unfile = File.substr(0, File.length() - ext->length() - 1); if (stat(unfile.c_str(),&Buf) == 0) { - FetchResult AltRes; AltRes.Size = Buf.st_size; AltRes.Filename = unfile; AltRes.LastModified = Buf.st_mtime; AltRes.IMSHit = false; if (Itm->LastModified == Buf.st_mtime && Itm->LastModified != 0) AltRes.IMSHit = true; - - URIDone(Res,&AltRes); - return true; + break; } // no break here as we could have situations similar to '.gz' vs '.tar.gz' here } } - if (Res.Filename.empty() == true) + if (Res.Filename.empty() == false) + { + Hashes Hash(Itm->ExpectedHashes); + FileFd Fd(Res.Filename, FileFd::ReadOnly); + Hash.AddFD(Fd); + Res.TakeHashes(Hash); + } + + if (AltRes.Filename.empty() == false) + URIDone(Res,&AltRes); + else if (Res.Filename.empty() == false) + URIDone(Res); + else return _error->Error(_("File not found")); - Hashes Hash(Itm->ExpectedHashes); - FileFd Fd(Res.Filename, FileFd::ReadOnly); - Hash.AddFD(Fd); - Res.TakeHashes(Hash); - URIDone(Res); return true; } /*}}}*/ -- cgit v1.2.3 From ff86d7df6a53ff6283de4b9a858c1dad98ed887f Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 15 Jun 2015 13:36:11 +0200 Subject: call URIStart in cdrom and file method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All other methods call it, so they should follow along even if the work they do afterwards is hardly breathtaking and usually results in a URIDone pretty soon, but the acquire system tells the individual item about this via a virtual method call, so even through none of our existing items contains any critical code in these, maybe one day they might. Consistency at least once… Which is also why this has a good sideeffect: file: and cdrom: requests appear now in the 'apt-get update' output. Finally - it never made sense to hide them for me. Okay, I guess it made before the new hit behavior, but now that you can actually see the difference in an update it makes sense to see if a file: repository changed or not as well. --- methods/cdrom.cc | 3 ++- methods/file.cc | 18 ++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) (limited to 'methods') diff --git a/methods/cdrom.cc b/methods/cdrom.cc index 10cb29f66..67265cfa3 100644 --- a/methods/cdrom.cc +++ b/methods/cdrom.cc @@ -260,7 +260,8 @@ bool CDROMMethod::Fetch(FetchItem *Itm) struct stat Buf; if (stat(Res.Filename.c_str(),&Buf) != 0) return _error->Error(_("File not found")); - + + URIStart(Res); if (NewID.empty() == false) CurrentID = NewID; Res.LastModified = Buf.st_mtime; diff --git a/methods/file.cc b/methods/file.cc index 5d5fffa67..5c76ec122 100644 --- a/methods/file.cc +++ b/methods/file.cc @@ -58,7 +58,10 @@ bool FileMethod::Fetch(FetchItem *Itm) { HashStringList const hsl = Itm->ExpectedHashes; if (Itm->ExpectedHashes.VerifyFile(File)) + { + Res.Filename = Itm->DestFile; Res.IMSHit = true; + } } } } @@ -78,7 +81,14 @@ bool FileMethod::Fetch(FetchItem *Itm) if (filesize != 0 && filesize == Res.Size) Res.IMSHit = true; } + + Hashes Hash(Itm->ExpectedHashes); + FileFd Fd(File, FileFd::ReadOnly); + Hash.AddFD(Fd); + Res.TakeHashes(Hash); } + if (Res.IMSHit == false) + URIStart(Res); // See if the uncompressed file exists and reuse it FetchResult AltRes; @@ -103,14 +113,6 @@ bool FileMethod::Fetch(FetchItem *Itm) } } - if (Res.Filename.empty() == false) - { - Hashes Hash(Itm->ExpectedHashes); - FileFd Fd(Res.Filename, FileFd::ReadOnly); - Hash.AddFD(Fd); - Res.TakeHashes(Hash); - } - if (AltRes.Filename.empty() == false) URIDone(Res,&AltRes); else if (Res.Filename.empty() == false) -- cgit v1.2.3