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 ++ 1 file changed, 2 insertions(+) (limited to 'methods/http.cc') 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); -- cgit v1.2.3-70-g09d2 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 --- cmdline/apt-helper.cc | 35 ++++-- methods/http.cc | 2 + methods/https.cc | 12 +- methods/server.cc | 26 +++-- methods/server.h | 5 +- test/integration/framework | 6 +- test/integration/test-apt-helper | 24 ++-- test/integration/test-partial-file-support | 62 +++++++++- test/interactive-helper/aptwebserver.cc | 181 ++++++++++++++++++----------- 9 files changed, 241 insertions(+), 112 deletions(-) (limited to 'methods/http.cc') diff --git a/cmdline/apt-helper.cc b/cmdline/apt-helper.cc index dd43ea1bc..63f70983c 100644 --- a/cmdline/apt-helper.cc +++ b/cmdline/apt-helper.cc @@ -48,23 +48,34 @@ static bool DoDownloadFile(CommandLine &CmdL) if (CmdL.FileSize() <= 2) return _error->Error(_("Must specify at least one pair url/filename")); - pkgAcquire Fetcher; AcqTextStatus Stat(ScreenWidth, _config->FindI("quiet",0)); Fetcher.Setup(&Stat); - std::string download_uri = CmdL.FileList[1]; - std::string targetfile = CmdL.FileList[2]; - std::string hash; - if (CmdL.FileSize() > 3) - hash = CmdL.FileList[3]; - // we use download_uri as descr and targetfile as short-descr - new pkgAcqFile(&Fetcher, download_uri, hash, 0, download_uri, targetfile, - "dest-dir-ignored", targetfile); - Fetcher.Run(); + + size_t fileind = 0; + std::vector targetfiles; + while (fileind + 2 <= CmdL.FileSize()) + { + std::string download_uri = CmdL.FileList[fileind + 1]; + std::string targetfile = CmdL.FileList[fileind + 2]; + std::string hash; + if (CmdL.FileSize() > fileind + 3) + hash = CmdL.FileList[fileind + 3]; + // we use download_uri as descr and targetfile as short-descr + new pkgAcqFile(&Fetcher, download_uri, hash, 0, download_uri, targetfile, + "dest-dir-ignored", targetfile); + targetfiles.push_back(targetfile); + fileind += 3; + } + bool Failed = false; - if (AcquireRun(Fetcher, 0, &Failed, NULL) == false || Failed == true || - FileExists(targetfile) == false) + if (AcquireRun(Fetcher, 0, &Failed, NULL) == false || Failed == true) return _error->Error(_("Download Failed")); + if (targetfiles.empty() == false) + for (std::vector::const_iterator f = targetfiles.begin(); f != targetfiles.end(); ++f) + if (FileExists(*f) == false) + return _error->Error(_("Download Failed")); + return true; } 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; diff --git a/test/integration/framework b/test/integration/framework index df1942ff9..ac482a7a0 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1064,8 +1064,8 @@ acquire::cdrom::autodetect 0;" > rootdir/etc/apt/apt.conf.d/00cdrom } downloadfile() { - local PROTO="$(echo "$1" | cut -d':' -f 1 )" - apthelper -o Debug::Acquire::${PROTO}=1 \ + local PROTO="${1%%:*}" + apthelper -o Debug::Acquire::${PROTO}=1 -o Debug::pkgAcquire::Worker=1 \ download-file "$1" "$2" 2>&1 || true # only if the file exists the download was successful if [ -e "$2" ]; then @@ -1221,7 +1221,7 @@ testsuccess() { msgtest 'Test for successful execution of' "$*" fi local OUTPUT="${TMPWORKINGDIRECTORY}/rootdir/tmp/testsuccess.output" - if $@ >${OUTPUT} 2>&1; then + if "$@" >${OUTPUT} 2>&1; then msgpass else echo >&2 diff --git a/test/integration/test-apt-helper b/test/integration/test-apt-helper index c749224ca..31e471677 100755 --- a/test/integration/test-apt-helper +++ b/test/integration/test-apt-helper @@ -10,34 +10,36 @@ configarchitecture "i386" changetohttpswebserver test_apt_helper_download() { - echo "foo" > aptarchive/foo + echo 'foo' > aptarchive/foo + echo 'bar' > aptarchive/foo2 msgtest 'apt-file download-file md5sum' - apthelper -qq download-file http://localhost:8080/foo foo2 MD5Sum:d3b07384d113edec49eaa6238ad5ff00 && msgpass || msgfail + testsuccess --nomsg apthelper download-file http://localhost:8080/foo foo2 MD5Sum:d3b07384d113edec49eaa6238ad5ff00 testfileequal foo2 'foo' msgtest 'apt-file download-file sha1' - apthelper -qq download-file http://localhost:8080/foo foo1 SHA1:f1d2d2f924e986ac86fdf7b36c94bcdf32beec15 && msgpass || msgfail + testsuccess --nomsg apthelper download-file http://localhost:8080/foo foo1 SHA1:f1d2d2f924e986ac86fdf7b36c94bcdf32beec15 testfileequal foo1 'foo' msgtest 'apt-file download-file sha256' - apthelper -qq download-file http://localhost:8080/foo foo3 SHA256:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c && msgpass || msgfail + testsuccess --nomsg apthelper download-file http://localhost:8080/foo foo3 SHA256:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c testfileequal foo3 'foo' msgtest 'apt-file download-file no-hash' - apthelper -qq download-file http://localhost:8080/foo foo4 && msgpass || msgfail + testsuccess --nomsg apthelper download-file http://localhost:8080/foo foo4 testfileequal foo4 'foo' msgtest 'apt-file download-file wrong hash' - if ! apthelper -qq download-file http://localhost:8080/foo foo5 MD5Sum:aabbcc 2>&1 2> download.stderr; then - msgpass - else - msgfail - fi - testfileequal download.stderr 'E: Failed to fetch http://localhost:8080/foo Hash Sum mismatch + testfailure --nomsg apthelper -qq download-file http://localhost:8080/foo foo5 MD5Sum:aabbcc + testfileequal rootdir/tmp/testfailure.output 'E: Failed to fetch http://localhost:8080/foo Hash Sum mismatch E: Download Failed' testfileequal foo5.FAILED 'foo' + + msgtest 'apt-file download-file md5sum sha1' + testsuccess --nomsg apthelper download-file http://localhost:8080/foo foo6 MD5Sum:d3b07384d113edec49eaa6238ad5ff00 http://localhost:8080/foo2 foo7 SHA1:e242ed3bffccdf271b7fbaf34ed72d089537b42f + testfileequal foo6 'foo' + testfileequal foo7 'bar' } test_apt_helper_detect_proxy() { diff --git a/test/integration/test-partial-file-support b/test/integration/test-partial-file-support index 5ab326def..160d451b6 100755 --- a/test/integration/test-partial-file-support +++ b/test/integration/test-partial-file-support @@ -24,13 +24,25 @@ testdownloadfile() { else msgpass fi - cat "$DOWNLOADLOG" | while read field hash; do + sed -e '/^ <- / s#%20# #g' -e '/^ <- / s#%0a#\n#g' "$DOWNLOADLOG" | grep '^.*-Hash: ' > receivedhashes.log + testsuccess test -s receivedhashes.log + local HASHES_OK=0 + local HASHES_BAD=0 + while read field hash; do local EXPECTED case "$field" in 'MD5Sum-Hash:') EXPECTED="$(md5sum "$TESTFILE" | cut -d' ' -f 1)";; 'SHA1-Hash:') EXPECTED="$(sha1sum "$TESTFILE" | cut -d' ' -f 1)";; 'SHA256-Hash:') EXPECTED="$(sha256sum "$TESTFILE" | cut -d' ' -f 1)";; 'SHA512-Hash:') EXPECTED="$(sha512sum "$TESTFILE" | cut -d' ' -f 1)";; + 'Checksum-FileSize-Hash:') + #filesize is too weak to check for != + if [ "$4" = '=' ]; then + EXPECTED="$(stat -c '%s' "$TESTFILE")" + else + continue + fi + ;; *) continue;; esac if [ "$4" = '=' ]; then @@ -40,15 +52,41 @@ testdownloadfile() { fi if [ "$EXPECTED" "$4" "$hash" ]; then msgpass + HASHES_OK=$((HASHES_OK+1)); else - cat >&2 "$DOWNLOADLOG" msgfail "expected: $EXPECTED ; got: $hash" + HASHES_BAD=$((HASHES_BAD+1)); fi - done + done < receivedhashes.log + msgtest 'At least one good hash and no bad ones' + if [ $HASHES_OK -eq 0 ] || [ $HASHES_BAD -ne 0 ]; then + cat >&2 "$DOWNLOADLOG" + msgfail + else + msgpass + fi } TESTFILE='aptarchive/testfile' cp -a ${TESTDIR}/framework $TESTFILE +cp -a ${TESTDIR}/framework "${TESTFILE}2" + +followuprequest() { + local DOWN='./testfile' + + copysource $TESTFILE 1M $DOWN + testdownloadfile 'completely downloaded file' "${1}/testfile" "$DOWN" '=' + testwebserverlaststatuscode '416' "$DOWNLOADLOG" + + copysource $TESTFILE 1M $DOWN + copysource "${TESTFILE}2" 20 "${DOWN}2" + msgtest 'Testing download of files with' 'completely downloaded file + partial file' + testsuccess --nomsg apthelper -o Debug::Acquire::${1%%:*}=1 -o Debug::pkgAcquire::Worker=1 \ + download-file "$1/testfile" "$DOWN" '' "$1/testfile2" "${DOWN}2" + testwebserverlaststatuscode '206' 'rootdir/tmp/testsuccess.output' + testsuccess diff -u "$TESTFILE" "${DOWN}" + testsuccess diff -u "${DOWN}" "${DOWN}2" +} testrun() { webserverconfig 'aptwebserver::support::range' 'true' @@ -65,9 +103,11 @@ testrun() { testdownloadfile 'invalid partial data' "${1}/testfile" './testfile' '!=' testwebserverlaststatuscode '206' "$DOWNLOADLOG" - copysource $TESTFILE 1M ./testfile - testdownloadfile 'completely downloaded file' "${1}/testfile" './testfile' '=' - testwebserverlaststatuscode '416' "$DOWNLOADLOG" + webserverconfig 'aptwebserver::closeOnError' 'false' + followuprequest "$1" + webserverconfig 'aptwebserver::closeOnError' 'true' + followuprequest "$1" + webserverconfig 'aptwebserver::closeOnError' 'false' copysource /dev/zero 1M ./testfile testdownloadfile 'too-big partial file' "${1}/testfile" './testfile' '=' @@ -85,8 +125,18 @@ testrun() { testwebserverlaststatuscode '200' "$DOWNLOADLOG" } +msgmsg 'http: Test with Content-Length' +webserverconfig 'aptwebserver::chunked-transfer-encoding' 'false' +testrun 'http://localhost:8080' +msgmsg 'http: Test with Transfer-Encoding: chunked' +webserverconfig 'aptwebserver::chunked-transfer-encoding' 'true' testrun 'http://localhost:8080' changetohttpswebserver +msgmsg 'https: Test with Content-Length' +webserverconfig 'aptwebserver::chunked-transfer-encoding' 'false' +testrun 'https://localhost:4433' +msgmsg 'https: Test with Transfer-Encoding: chunked' +webserverconfig 'aptwebserver::chunked-transfer-encoding' 'true' testrun 'https://localhost:4433' diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc index 34476e1af..cd52da692 100644 --- a/test/interactive-helper/aptwebserver.cc +++ b/test/interactive-helper/aptwebserver.cc @@ -19,6 +19,8 @@ #include #include #include + +#include #include #include #include @@ -79,12 +81,21 @@ static char const * httpcodeToStr(int const httpcode) /*{{{*/ return NULL; } /*}}}*/ +static bool chunkedTransferEncoding(std::list const &headers) { + if (std::find(headers.begin(), headers.end(), "Transfer-Encoding: chunked") != headers.end()) + return true; + if (_config->FindB("aptwebserver::chunked-transfer-encoding", false) == true) + return true; + return false; +} static void addFileHeaders(std::list &headers, FileFd &data)/*{{{*/ { - std::ostringstream contentlength; - contentlength << "Content-Length: " << data.FileSize(); - headers.push_back(contentlength.str()); - + if (chunkedTransferEncoding(headers) == false) + { + std::ostringstream contentlength; + contentlength << "Content-Length: " << data.FileSize(); + headers.push_back(contentlength.str()); + } std::string lastmodified("Last-Modified: "); lastmodified.append(TimeRFC1123(data.ModificationTime())); headers.push_back(lastmodified); @@ -92,9 +103,12 @@ static void addFileHeaders(std::list &headers, FileFd &data)/*{{{*/ /*}}}*/ static void addDataHeaders(std::list &headers, std::string &data)/*{{{*/ { - std::ostringstream contentlength; - contentlength << "Content-Length: " << data.size(); - headers.push_back(contentlength.str()); + if (chunkedTransferEncoding(headers) == false) + { + std::ostringstream contentlength; + contentlength << "Content-Length: " << data.size(); + headers.push_back(contentlength.str()); + } } /*}}}*/ static bool sendHead(int const client, int const httpcode, std::list &headers)/*{{{*/ @@ -114,6 +128,9 @@ static bool sendHead(int const client, int const httpcode, std::list>> RESPONSE to " << client << " >>>" << std::endl; bool Success = true; for (std::list::const_iterator h = headers.begin(); @@ -130,25 +147,55 @@ static bool sendHead(int const client, int const httpcode, std::list const &headers, FileFd &data)/*{{{*/ { bool Success = true; + bool const chunked = chunkedTransferEncoding(headers); char buffer[500]; unsigned long long actual = 0; while ((Success &= data.Read(buffer, sizeof(buffer), &actual)) == true) { if (actual == 0) break; - Success &= FileFd::Write(client, buffer, actual); + + if (chunked == true) + { + std::string size; + strprintf(size, "%llX\r\n", actual); + Success &= FileFd::Write(client, size.c_str(), size.size()); + Success &= FileFd::Write(client, buffer, actual); + Success &= FileFd::Write(client, "\r\n", strlen("\r\n")); + } + else + Success &= FileFd::Write(client, buffer, actual); + } + if (chunked == true) + { + char const * const finish = "0\r\n\r\n"; + Success &= FileFd::Write(client, finish, strlen(finish)); } if (Success == false) - std::cerr << "SENDFILE: READ/WRITE ERROR to " << client << std::endl; + std::cerr << "SENDFILE:" << (chunked ? " CHUNKED" : "") << " READ/WRITE ERROR to " << client << std::endl; return Success; } /*}}}*/ -static bool sendData(int const client, std::string const &data) /*{{{*/ +static bool sendData(int const client, std::list const &headers, std::string const &data)/*{{{*/ { - if (FileFd::Write(client, data.c_str(), data.size()) == false) + if (chunkedTransferEncoding(headers) == true) + { + unsigned long long const ullsize = data.length(); + std::string size; + strprintf(size, "%llX\r\n", ullsize); + char const * const finish = "\r\n0\r\n\r\n"; + if (FileFd::Write(client, size.c_str(), size.length()) == false || + FileFd::Write(client, data.c_str(), ullsize) == false || + FileFd::Write(client, finish, strlen(finish)) == false) + { + std::cerr << "SENDDATA: CHUNK WRITE ERROR to " << client << std::endl; + return false; + } + } + else if (FileFd::Write(client, data.c_str(), data.size()) == false) { std::cerr << "SENDDATA: WRITE ERROR to " << client << std::endl; return false; @@ -157,34 +204,38 @@ static bool sendData(int const client, std::string const &data) /*{{{*/ } /*}}}*/ static void sendError(int const client, int const httpcode, std::string const &request,/*{{{*/ - bool content, std::string const &error = "") + bool const content, std::string const &error, std::list &headers) { - std::list headers; std::string response(""); response.append(httpcodeToStr(httpcode)).append(""); response.append("

").append(httpcodeToStr(httpcode)).append("

"); if (httpcode != 200) - { - if (error.empty() == false) - response.append("

Error: ").append(error).append("

"); - response.append("This error is a result of the request:
");
-   }
+      response.append("

Error: "); + else + response.append("

Success: "); + if (error.empty() == false) + response.append(error); + else + response.append(httpcodeToStr(httpcode)); + if (httpcode != 200) + response.append("

This error is a result of the request:
");
    else
-   {
-      if (error.empty() == false)
-	 response.append("

Success: ").append(error).append("

"); response.append("The successfully executed operation was requested by:
");
-   }
    response.append(request).append("
"); + if (httpcode != 200) + { + if (_config->FindB("aptwebserver::closeOnError", false) == true) + headers.push_back("Connection: close"); + } addDataHeaders(headers, response); sendHead(client, httpcode, headers); if (content == true) - sendData(client, response); + sendData(client, headers, response); } static void sendSuccess(int const client, std::string const &request, - bool content, std::string const &error = "") + bool const content, std::string const &error, std::list &headers) { - sendError(client, 200, request, content, error); + sendError(client, 200, request, content, error, headers); } /*}}}*/ static void sendRedirect(int const client, int const httpcode, std::string const &uri,/*{{{*/ @@ -221,7 +272,7 @@ static void sendRedirect(int const client, int const httpcode, std::string const headers.push_back(location); sendHead(client, httpcode, headers); if (content == true) - sendData(client, response); + sendData(client, headers, response); } /*}}}*/ static int filter_hidden_files(const struct dirent *a) /*{{{*/ @@ -263,16 +314,15 @@ static int grouped_alpha_case_sort(const struct dirent **a, const struct dirent } /*}}}*/ static void sendDirectoryListing(int const client, std::string const &dir,/*{{{*/ - std::string const &request, bool content) + std::string const &request, bool content, std::list &headers) { - std::list headers; std::ostringstream listing; struct dirent **namelist; int const counter = scandir(dir.c_str(), &namelist, filter_hidden_files, grouped_alpha_case_sort); if (counter == -1) { - sendError(client, 500, request, content); + sendError(client, 500, request, content, "scandir failed", headers); return; } @@ -311,18 +361,18 @@ static void sendDirectoryListing(int const client, std::string const &dir,/*{{{* addDataHeaders(headers, response); sendHead(client, 200, headers); if (content == true) - sendData(client, response); + sendData(client, headers, response); } /*}}}*/ static bool parseFirstLine(int const client, std::string const &request,/*{{{*/ std::string &filename, std::string ¶ms, bool &sendContent, - bool &closeConnection) + bool &closeConnection, std::list &headers) { if (strncmp(request.c_str(), "HEAD ", 5) == 0) sendContent = false; if (strncmp(request.c_str(), "GET ", 4) != 0) { - sendError(client, 501, request, true); + sendError(client, 501, request, true, "", headers); return false; } @@ -333,7 +383,7 @@ static bool parseFirstLine(int const client, std::string const &request,/*{{{*/ if (lineend == std::string::npos || filestart == std::string::npos || fileend == std::string::npos || filestart == fileend) { - sendError(client, 500, request, sendContent, "Filename can't be extracted"); + sendError(client, 500, request, sendContent, "Filename can't be extracted", headers); return false; } @@ -345,14 +395,14 @@ static bool parseFirstLine(int const client, std::string const &request,/*{{{*/ closeConnection = strcasecmp(LookupTag(request, "Connection", "Keep-Alive").c_str(), "close") == 0; else { - sendError(client, 500, request, sendContent, "Not a HTTP/1.{0,1} request"); + sendError(client, 500, request, sendContent, "Not a HTTP/1.{0,1} request", headers); return false; } filename = request.substr(filestart, fileend - filestart); if (filename.find(' ') != std::string::npos) { - sendError(client, 500, request, sendContent, "Filename contains an unencoded space"); + sendError(client, 500, request, sendContent, "Filename contains an unencoded space", headers); return false; } @@ -360,7 +410,7 @@ static bool parseFirstLine(int const client, std::string const &request,/*{{{*/ if (host.empty() == true) { // RFC 2616 §14.23 requires Host - sendError(client, 400, request, sendContent, "Host header is required"); + sendError(client, 400, request, sendContent, "Host header is required", headers); return false; } host = "http://" + host; @@ -371,7 +421,7 @@ static bool parseFirstLine(int const client, std::string const &request,/*{{{*/ { if (absolute.find("uri") == std::string::npos) { - sendError(client, 400, request, sendContent, "Request is absoluteURI, but configured to not accept that"); + sendError(client, 400, request, sendContent, "Request is absoluteURI, but configured to not accept that", headers); return false; } // strip the host from the request to make it an absolute path @@ -379,7 +429,7 @@ static bool parseFirstLine(int const client, std::string const &request,/*{{{*/ } else if (absolute.find("path") == std::string::npos) { - sendError(client, 400, request, sendContent, "Request is absolutePath, but configured to not accept that"); + sendError(client, 400, request, sendContent, "Request is absolutePath, but configured to not accept that", headers); return false; } @@ -398,7 +448,8 @@ static bool parseFirstLine(int const client, std::string const &request,/*{{{*/ filename.find_first_of("\r\n\t\f\v") != std::string::npos || filename.find("/../") != std::string::npos) { - sendError(client, 400, request, sendContent, "Filename contains illegal character (sequence)"); + std::list headers; + sendError(client, 400, request, sendContent, "Filename contains illegal character (sequence)", headers); return false; } @@ -434,46 +485,45 @@ static bool parseFirstLine(int const client, std::string const &request,/*{{{*/ return true; } /*}}}*/ -static bool handleOnTheFlyReconfiguration(int const client, std::string const &request, std::vector const &parts)/*{{{*/ +static bool handleOnTheFlyReconfiguration(int const client, std::string const &request,/*{{{*/ + std::vector parts, std::list &headers) { size_t const pcount = parts.size(); if (pcount == 4 && parts[1] == "set") { _config->Set(parts[2], parts[3]); - sendSuccess(client, request, true, "Option '" + parts[2] + "' was set to '" + parts[3] + "'!"); + sendSuccess(client, request, true, "Option '" + parts[2] + "' was set to '" + parts[3] + "'!", headers); return true; } else if (pcount == 4 && parts[1] == "find") { - std::list headers; std::string response = _config->Find(parts[2], parts[3]); addDataHeaders(headers, response); sendHead(client, 200, headers); - sendData(client, response); + sendData(client, headers, response); return true; } else if (pcount == 3 && parts[1] == "find") { - std::list headers; if (_config->Exists(parts[2]) == true) { std::string response = _config->Find(parts[2]); addDataHeaders(headers, response); sendHead(client, 200, headers); - sendData(client, response); + sendData(client, headers, response); return true; } - sendError(client, 404, request, "Requested Configuration option doesn't exist."); + sendError(client, 404, request, true, "Requested Configuration option doesn't exist", headers); return false; } else if (pcount == 3 && parts[1] == "clear") { _config->Clear(parts[2]); - sendSuccess(client, request, true, "Option '" + parts[2] + "' was cleared."); + sendSuccess(client, request, true, "Option '" + parts[2] + "' was cleared.", headers); return true; } - sendError(client, 400, request, true, "Unknown on-the-fly configuration request"); + sendError(client, 400, request, true, "Unknown on-the-fly configuration request", headers); return false; } /*}}}*/ @@ -482,18 +532,22 @@ static void * handleClient(void * voidclient) /*{{{*/ int client = *((int*)(voidclient)); std::clog << "ACCEPT client " << client << std::endl; std::vector messages; - while (ReadMessages(client, messages)) + bool closeConnection = false; + std::list headers; + while (closeConnection == false && ReadMessages(client, messages)) { - bool closeConnection = false; + // if we announced a closing, do the close + if (std::find(headers.begin(), headers.end(), std::string("Connection: close")) != headers.end()) + break; + headers.clear(); for (std::vector::const_iterator m = messages.begin(); m != messages.end() && closeConnection == false; ++m) { std::clog << ">>> REQUEST from " << client << " >>>" << std::endl << *m << std::endl << "<<<<<<<<<<<<<<<<" << std::endl; - std::list headers; std::string filename; std::string params; bool sendContent = true; - if (parseFirstLine(client, *m, filename, params, sendContent, closeConnection) == false) + if (parseFirstLine(client, *m, filename, params, sendContent, closeConnection, headers) == false) continue; // special webserver command request @@ -502,7 +556,7 @@ static void * handleClient(void * voidclient) /*{{{*/ std::vector parts = VectorizeString(filename, '/'); if (parts[0] == "_config") { - handleOnTheFlyReconfiguration(client, *m, parts); + handleOnTheFlyReconfiguration(client, *m, parts, headers); continue; } } @@ -534,7 +588,7 @@ static void * handleClient(void * voidclient) /*{{{*/ { char error[300]; regerror(res, pattern, error, sizeof(error)); - sendError(client, 500, *m, sendContent, error); + sendError(client, 500, *m, sendContent, error, headers); continue; } if (regexec(pattern, filename.c_str(), 0, 0, 0) == 0) @@ -553,7 +607,7 @@ static void * handleClient(void * voidclient) /*{{{*/ if (_config->FindB("aptwebserver::support::http", true) == false && LookupTag(*m, "Host").find(":4433") == std::string::npos) { - sendError(client, 400, *m, sendContent, "HTTP disabled, all requests must be HTTPS"); + sendError(client, 400, *m, sendContent, "HTTP disabled, all requests must be HTTPS", headers); continue; } else if (RealFileExists(filename) == true) @@ -609,17 +663,16 @@ static void * handleClient(void * voidclient) /*{{{*/ headers.push_back(contentrange.str()); sendHead(client, 206, headers); if (sendContent == true) - sendFile(client, data); + sendFile(client, headers, data); continue; } else { - headers.push_back("Content-Length: 0"); std::ostringstream contentrange; contentrange << "Content-Range: bytes */" << filesize; headers.push_back(contentrange.str()); - sendHead(client, 416, headers); - continue; + sendError(client, 416, *m, sendContent, "", headers); + break; } } } @@ -628,22 +681,20 @@ static void * handleClient(void * voidclient) /*{{{*/ addFileHeaders(headers, data); sendHead(client, 200, headers); if (sendContent == true) - sendFile(client, data); + sendFile(client, headers, data); } else if (DirectoryExists(filename) == true) { if (filename[filename.length()-1] == '/') - sendDirectoryListing(client, filename, *m, sendContent); + sendDirectoryListing(client, filename, *m, sendContent, headers); else sendRedirect(client, 301, filename.append("/"), *m, sendContent); } else - sendError(client, 404, *m, sendContent); + sendError(client, 404, *m, sendContent, "", headers); } _error->DumpErrors(std::cerr); messages.clear(); - if (closeConnection == true) - break; } close(client); std::clog << "CLOSE client " << client << std::endl; -- cgit v1.2.3-70-g09d2 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 +- .../test-bug-778375-server-has-no-reason-phrase | 40 ++++++++++ test/interactive-helper/aptwebserver.cc | 88 +++++++++++----------- 7 files changed, 124 insertions(+), 69 deletions(-) create mode 100755 test/integration/test-bug-778375-server-has-no-reason-phrase (limited to 'methods/http.cc') 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; diff --git a/test/integration/test-bug-778375-server-has-no-reason-phrase b/test/integration/test-bug-778375-server-has-no-reason-phrase new file mode 100755 index 000000000..23481ef88 --- /dev/null +++ b/test/integration/test-bug-778375-server-has-no-reason-phrase @@ -0,0 +1,40 @@ +#!/bin/sh +set -e + +TESTDIR=$(readlink -f $(dirname $0)) +. $TESTDIR/framework + +setupenvironment +configarchitecture 'native' + +echo 'found' > aptarchive/working +changetohttpswebserver -o 'aptwebserver::redirect::replace::/redirectme/=/' \ + -o 'aptwebserver::httpcode::200=200' -o 'aptwebserver::httpcode::404=404' \ + -o 'aptwebserver::httpcode::301=301' + +testdownload() { + rm -f downfile + msgtest "download of a $1 via" "${3%%:*}" + $2 --nomsg downloadfile "$3" downfile + + cp rootdir/tmp/testsuccess.output download.log + #looking for "HTTP server doesn't give Reason-Phrase for 200" + testsuccess grep 'give Reason-Phrase for' download.log + + if [ "$2" = 'testsuccess' ]; then + testfileequal downfile 'found' + else + testfailure test -e downfile + fi +} + +runtest() { + testdownload 'file works' 'testsuccess' "$1/working" + testdownload 'file via redirect works' 'testsuccess' "$1/redirectme/working" + + testdownload 'non-existent file fails' 'testfailure' "$1/failing" + testdownload 'non-existent file via redirect fails' 'testfailure' "$1/redirectme/failing" +} + +runtest 'http://localhost:8080' +runtest 'https://localhost:4433' diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc index 3403bbdd2..644629a33 100644 --- a/test/interactive-helper/aptwebserver.cc +++ b/test/interactive-helper/aptwebserver.cc @@ -27,58 +27,58 @@ #include #include -static char const * httpcodeToStr(int const httpcode) /*{{{*/ +static std::string httpcodeToStr(int const httpcode) /*{{{*/ { switch (httpcode) { // Informational 1xx - case 100: return "100 Continue"; - case 101: return "101 Switching Protocols"; + case 100: return _config->Find("aptwebserver::httpcode::100", "100 Continue"); + case 101: return _config->Find("aptwebserver::httpcode::101", "101 Switching Protocols"); // Successful 2xx - case 200: return "200 OK"; - case 201: return "201 Created"; - case 202: return "202 Accepted"; - case 203: return "203 Non-Authoritative Information"; - case 204: return "204 No Content"; - case 205: return "205 Reset Content"; - case 206: return "206 Partial Content"; + case 200: return _config->Find("aptwebserver::httpcode::200", "200 OK"); + case 201: return _config->Find("aptwebserver::httpcode::201", "201 Created"); + case 202: return _config->Find("aptwebserver::httpcode::202", "202 Accepted"); + case 203: return _config->Find("aptwebserver::httpcode::203", "203 Non-Authoritative Information"); + case 204: return _config->Find("aptwebserver::httpcode::204", "204 No Content"); + case 205: return _config->Find("aptwebserver::httpcode::205", "205 Reset Content"); + case 206: return _config->Find("aptwebserver::httpcode::206", "206 Partial Content"); // Redirections 3xx - case 300: return "300 Multiple Choices"; - case 301: return "301 Moved Permanently"; - case 302: return "302 Found"; - case 303: return "303 See Other"; - case 304: return "304 Not Modified"; - case 305: return "304 Use Proxy"; - case 307: return "307 Temporary Redirect"; + case 300: return _config->Find("aptwebserver::httpcode::300", "300 Multiple Choices"); + case 301: return _config->Find("aptwebserver::httpcode::301", "301 Moved Permanently"); + case 302: return _config->Find("aptwebserver::httpcode::302", "302 Found"); + case 303: return _config->Find("aptwebserver::httpcode::303", "303 See Other"); + case 304: return _config->Find("aptwebserver::httpcode::304", "304 Not Modified"); + case 305: return _config->Find("aptwebserver::httpcode::305", "305 Use Proxy"); + case 307: return _config->Find("aptwebserver::httpcode::307", "307 Temporary Redirect"); // Client errors 4xx - case 400: return "400 Bad Request"; - case 401: return "401 Unauthorized"; - case 402: return "402 Payment Required"; - case 403: return "403 Forbidden"; - case 404: return "404 Not Found"; - case 405: return "405 Method Not Allowed"; - case 406: return "406 Not Acceptable"; - case 407: return "407 Proxy Authentication Required"; - case 408: return "408 Request Time-out"; - case 409: return "409 Conflict"; - case 410: return "410 Gone"; - case 411: return "411 Length Required"; - case 412: return "412 Precondition Failed"; - case 413: return "413 Request Entity Too Large"; - case 414: return "414 Request-URI Too Large"; - case 415: return "415 Unsupported Media Type"; - case 416: return "416 Requested range not satisfiable"; - case 417: return "417 Expectation Failed"; - case 418: return "418 I'm a teapot"; + case 400: return _config->Find("aptwebserver::httpcode::400", "400 Bad Request"); + case 401: return _config->Find("aptwebserver::httpcode::401", "401 Unauthorized"); + case 402: return _config->Find("aptwebserver::httpcode::402", "402 Payment Required"); + case 403: return _config->Find("aptwebserver::httpcode::403", "403 Forbidden"); + case 404: return _config->Find("aptwebserver::httpcode::404", "404 Not Found"); + case 405: return _config->Find("aptwebserver::httpcode::405", "405 Method Not Allowed"); + case 406: return _config->Find("aptwebserver::httpcode::406", "406 Not Acceptable"); + case 407: return _config->Find("aptwebserver::httpcode::407", "407 Proxy Authentication Required"); + case 408: return _config->Find("aptwebserver::httpcode::408", "408 Request Time-out"); + case 409: return _config->Find("aptwebserver::httpcode::409", "409 Conflict"); + case 410: return _config->Find("aptwebserver::httpcode::410", "410 Gone"); + case 411: return _config->Find("aptwebserver::httpcode::411", "411 Length Required"); + case 412: return _config->Find("aptwebserver::httpcode::412", "412 Precondition Failed"); + case 413: return _config->Find("aptwebserver::httpcode::413", "413 Request Entity Too Large"); + case 414: return _config->Find("aptwebserver::httpcode::414", "414 Request-URI Too Large"); + case 415: return _config->Find("aptwebserver::httpcode::415", "415 Unsupported Media Type"); + case 416: return _config->Find("aptwebserver::httpcode::416", "416 Requested range not satisfiable"); + case 417: return _config->Find("aptwebserver::httpcode::417", "417 Expectation Failed"); + case 418: return _config->Find("aptwebserver::httpcode::418", "418 I'm a teapot"); // Server error 5xx - case 500: return "500 Internal Server Error"; - case 501: return "501 Not Implemented"; - case 502: return "502 Bad Gateway"; - case 503: return "503 Service Unavailable"; - case 504: return "504 Gateway Time-out"; - case 505: return "505 HTTP Version not supported"; - } - return NULL; + case 500: return _config->Find("aptwebserver::httpcode::500", "500 Internal Server Error"); + case 501: return _config->Find("aptwebserver::httpcode::501", "501 Not Implemented"); + case 502: return _config->Find("aptwebserver::httpcode::502", "502 Bad Gateway"); + case 503: return _config->Find("aptwebserver::httpcode::503", "503 Service Unavailable"); + case 504: return _config->Find("aptwebserver::httpcode::504", "504 Gateway Time-out"); + case 505: return _config->Find("aptwebserver::httpcode::505", "505 HTTP Version not supported"); + } + return ""; } /*}}}*/ static bool chunkedTransferEncoding(std::list const &headers) { -- cgit v1.2.3-70-g09d2 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 +++--- test/interactive-helper/aptwebserver.cc | 28 +++++++++++++++++++++------- 2 files changed, 24 insertions(+), 10 deletions(-) (limited to 'methods/http.cc') 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; diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc index 644629a33..86d5c06f0 100644 --- a/test/interactive-helper/aptwebserver.cc +++ b/test/interactive-helper/aptwebserver.cc @@ -598,17 +598,24 @@ static void * handleClient(void * voidclient) /*{{{*/ { int client = *((int*)(voidclient)); std::clog << "ACCEPT client " << client << std::endl; - std::vector messages; bool closeConnection = false; - std::list headers; - while (closeConnection == false && ReadMessages(client, messages)) + while (closeConnection == false) { - // if we announced a closing, do the close - if (std::find(headers.begin(), headers.end(), std::string("Connection: close")) != headers.end()) + std::vector messages; + if (ReadMessages(client, messages) == false) break; - headers.clear(); + + std::list headers; for (std::vector::const_iterator m = messages.begin(); m != messages.end() && closeConnection == false; ++m) { + // if we announced a closing in previous response, do the close now + if (std::find(headers.begin(), headers.end(), std::string("Connection: close")) != headers.end()) + { + closeConnection = true; + break; + } + headers.clear(); + std::clog << ">>> REQUEST from " << client << " >>>" << std::endl << *m << std::endl << "<<<<<<<<<<<<<<<<" << std::endl; std::string filename; @@ -760,9 +767,16 @@ static void * handleClient(void * voidclient) /*{{{*/ else sendError(client, 404, *m, sendContent, "", headers); } + + // if we announced a closing in the last response, do the close now + if (std::find(headers.begin(), headers.end(), std::string("Connection: close")) != headers.end()) + closeConnection = true; + + if (_error->PendingError() == true) + break; _error->DumpErrors(std::cerr); - messages.clear(); } + _error->DumpErrors(std::cerr); close(client); std::clog << "CLOSE client " << client << std::endl; return NULL; -- cgit v1.2.3-70-g09d2 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. --- apt-pkg/contrib/hashes.cc | 63 +++++++++++++++++++++++++++++++++----------- apt-pkg/contrib/hashes.h | 21 ++++++++++++--- ftparchive/cachedb.cc | 4 +-- ftparchive/writer.cc | 4 +-- 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 +- test/libapt/hashsums_test.cc | 24 +++++++++++++++++ 18 files changed, 116 insertions(+), 46 deletions(-) (limited to 'methods/http.cc') diff --git a/apt-pkg/contrib/hashes.cc b/apt-pkg/contrib/hashes.cc index 6e7080bc9..953465091 100644 --- a/apt-pkg/contrib/hashes.cc +++ b/apt-pkg/contrib/hashes.cc @@ -250,28 +250,34 @@ bool HashStringList::operator!=(HashStringList const &other) const class PrivateHashes { public: unsigned long long FileSize; + unsigned int CalcHashes; - PrivateHashes() : FileSize(0) {} + PrivateHashes(unsigned int const CalcHashes) : FileSize(0), CalcHashes(CalcHashes) {} }; /*}}}*/ // Hashes::Add* - Add the contents of data or FD /*{{{*/ -bool Hashes::Add(const unsigned char * const Data,unsigned long long const Size, unsigned int const Hashes) +bool Hashes::Add(const unsigned char * const Data, unsigned long long const Size) { bool Res = true; APT_IGNORE_DEPRECATED_PUSH - if ((Hashes & MD5SUM) == MD5SUM) + if ((d->CalcHashes & MD5SUM) == MD5SUM) Res &= MD5.Add(Data, Size); - if ((Hashes & SHA1SUM) == SHA1SUM) + if ((d->CalcHashes & SHA1SUM) == SHA1SUM) Res &= SHA1.Add(Data, Size); - if ((Hashes & SHA256SUM) == SHA256SUM) + if ((d->CalcHashes & SHA256SUM) == SHA256SUM) Res &= SHA256.Add(Data, Size); - if ((Hashes & SHA512SUM) == SHA512SUM) + if ((d->CalcHashes & SHA512SUM) == SHA512SUM) Res &= SHA512.Add(Data, Size); APT_IGNORE_DEPRECATED_POP d->FileSize += Size; return Res; } -bool Hashes::AddFD(int const Fd,unsigned long long Size, unsigned int const Hashes) +bool Hashes::Add(const unsigned char * const Data, unsigned long long const Size, unsigned int const Hashes) +{ + d->CalcHashes = Hashes; + return Add(Data, Size); +} +bool Hashes::AddFD(int const Fd,unsigned long long Size) { unsigned char Buf[64*64]; bool const ToEOF = (Size == UntilEOF); @@ -285,12 +291,17 @@ bool Hashes::AddFD(int const Fd,unsigned long long Size, unsigned int const Hash if (ToEOF && Res == 0) // EOF break; Size -= Res; - if (Add(Buf, Res, Hashes) == false) + if (Add(Buf, Res) == false) return false; } return true; } -bool Hashes::AddFD(FileFd &Fd,unsigned long long Size, unsigned int const Hashes) +bool Hashes::AddFD(int const Fd,unsigned long long Size, unsigned int const Hashes) +{ + d->CalcHashes = Hashes; + return AddFD(Fd, Size); +} +bool Hashes::AddFD(FileFd &Fd,unsigned long long Size) { unsigned char Buf[64*64]; bool const ToEOF = (Size == 0); @@ -309,20 +320,29 @@ bool Hashes::AddFD(FileFd &Fd,unsigned long long Size, unsigned int const Hashes else if (a == 0) // EOF break; Size -= a; - if (Add(Buf, a, Hashes) == false) + if (Add(Buf, a) == false) return false; } return true; +} +bool Hashes::AddFD(FileFd &Fd,unsigned long long Size, unsigned int const Hashes) +{ + d->CalcHashes = Hashes; + return AddFD(Fd, Size); } /*}}}*/ HashStringList Hashes::GetHashStringList() { HashStringList hashes; APT_IGNORE_DEPRECATED_PUSH - hashes.push_back(HashString("MD5Sum", MD5.Result().Value())); - hashes.push_back(HashString("SHA1", SHA1.Result().Value())); - hashes.push_back(HashString("SHA256", SHA256.Result().Value())); - hashes.push_back(HashString("SHA512", SHA512.Result().Value())); + if ((d->CalcHashes & MD5SUM) == MD5SUM) + hashes.push_back(HashString("MD5Sum", MD5.Result().Value())); + if ((d->CalcHashes & SHA1SUM) == SHA1SUM) + hashes.push_back(HashString("SHA1", SHA1.Result().Value())); + if ((d->CalcHashes & SHA256SUM) == SHA256SUM) + hashes.push_back(HashString("SHA256", SHA256.Result().Value())); + if ((d->CalcHashes & SHA512SUM) == SHA512SUM) + hashes.push_back(HashString("SHA512", SHA512.Result().Value())); APT_IGNORE_DEPRECATED_POP std::string SizeStr; strprintf(SizeStr, "%llu", d->FileSize); @@ -330,6 +350,19 @@ APT_IGNORE_DEPRECATED_POP return hashes; } APT_IGNORE_DEPRECATED_PUSH -Hashes::Hashes() { d = new PrivateHashes(); } +Hashes::Hashes() { d = new PrivateHashes(~0); } +Hashes::Hashes(unsigned int const Hashes) { d = new PrivateHashes(Hashes); } +Hashes::Hashes(HashStringList const &Hashes) { + unsigned int calcHashes = Hashes.usable() ? 0 : ~0; + if (Hashes.find("MD5Sum") != NULL) + calcHashes |= MD5SUM; + if (Hashes.find("SHA1") != NULL) + calcHashes |= SHA1SUM; + if (Hashes.find("SHA256") != NULL) + calcHashes |= SHA256SUM; + if (Hashes.find("SHA512") != NULL) + calcHashes |= SHA512SUM; + d = new PrivateHashes(calcHashes); +} Hashes::~Hashes() { delete d; } APT_IGNORE_DEPRECATED_POP diff --git a/apt-pkg/contrib/hashes.h b/apt-pkg/contrib/hashes.h index 154862457..ac13c8ace 100644 --- a/apt-pkg/contrib/hashes.h +++ b/apt-pkg/contrib/hashes.h @@ -178,7 +178,8 @@ class Hashes static const int UntilEOF = 0; - bool Add(const unsigned char * const Data, unsigned long long const Size, unsigned int const Hashes = ~0); + bool Add(const unsigned char * const Data, unsigned long long const Size); + APT_DEPRECATED bool Add(const unsigned char * const Data, unsigned long long const Size, unsigned int const Hashes); inline bool Add(const char * const Data) {return Add((unsigned char const * const)Data,strlen(Data));}; inline bool Add(const unsigned char * const Beg,const unsigned char * const End) @@ -186,13 +187,24 @@ class Hashes enum SupportedHashes { MD5SUM = (1 << 0), SHA1SUM = (1 << 1), SHA256SUM = (1 << 2), SHA512SUM = (1 << 3) }; - bool AddFD(int const Fd,unsigned long long Size = 0, unsigned int const Hashes = ~0); - bool AddFD(FileFd &Fd,unsigned long long Size = 0, unsigned int const Hashes = ~0); + bool AddFD(int const Fd,unsigned long long Size = 0); + APT_DEPRECATED bool AddFD(int const Fd,unsigned long long Size, unsigned int const Hashes); + bool AddFD(FileFd &Fd,unsigned long long Size = 0); + APT_DEPRECATED bool AddFD(FileFd &Fd,unsigned long long Size, unsigned int const Hashes); HashStringList GetHashStringList(); APT_IGNORE_DEPRECATED_PUSH + /** create a Hashes object to calculate all supported hashes + * + * If ALL is too much, you can limit which Hashes are calculated + * with the following other constructors which mention explicitly + * which hashes to generate. */ Hashes(); + /** @param Hashes bitflag composed of #SupportedHashes */ + Hashes(unsigned int const Hashes); + /** @param Hashes is a list of hashes */ + Hashes(HashStringList const &Hashes); virtual ~Hashes(); APT_IGNORE_DEPRECATED_POP @@ -208,15 +220,16 @@ APT_IGNORE_DEPRECATED_POP } public: +APT_IGNORE_DEPRECATED_PUSH APT_DEPRECATED bool AddFD(int const Fd, unsigned long long Size, bool const addMD5, bool const addSHA1, bool const addSHA256, bool const addSHA512) { return AddFD(Fd, Size, boolsToFlag(addMD5, addSHA1, addSHA256, addSHA512)); }; - APT_DEPRECATED bool AddFD(FileFd &Fd, unsigned long long Size, bool const addMD5, bool const addSHA1, bool const addSHA256, bool const addSHA512) { return AddFD(Fd, Size, boolsToFlag(addMD5, addSHA1, addSHA256, addSHA512)); }; +APT_IGNORE_DEPRECATED_POP }; #endif diff --git a/ftparchive/cachedb.cc b/ftparchive/cachedb.cc index 1dc268594..cc3527ea4 100644 --- a/ftparchive/cachedb.cc +++ b/ftparchive/cachedb.cc @@ -441,8 +441,8 @@ bool CacheDB::GetHashes(bool const GenOnly, unsigned int const DoHashes) if (OpenFile() == false) return false; - Hashes hashes; - if (Fd->Seek(0) == false || hashes.AddFD(*Fd, CurStat.FileSize, FlHashes) == false) + Hashes hashes(FlHashes); + if (Fd->Seek(0) == false || hashes.AddFD(*Fd, CurStat.FileSize) == false) return false; HashStringList hl = hashes.GetHashStringList(); diff --git a/ftparchive/writer.cc b/ftparchive/writer.cc index db68c21f0..593278590 100644 --- a/ftparchive/writer.cc +++ b/ftparchive/writer.cc @@ -1075,8 +1075,8 @@ bool ReleaseWriter::DoPackage(string FileName) CheckSums[NewFileName].size = fd.Size(); - Hashes hs; - hs.AddFD(fd, 0, DoHashes); + Hashes hs(DoHashes); + hs.AddFD(fd); CheckSums[NewFileName].Hashes = hs.GetHashStringList(); fd.Close(); 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; diff --git a/test/libapt/hashsums_test.cc b/test/libapt/hashsums_test.cc index a19a0befd..edcd8a11a 100644 --- a/test/libapt/hashsums_test.cc +++ b/test/libapt/hashsums_test.cc @@ -193,6 +193,30 @@ TEST(HashSumsTest, FileBased) EXPECT_EQ(FileSize, list.find("Checksum-FileSize")->HashValue()); } fd.Seek(0); + { + Hashes hashes(Hashes::MD5SUM | Hashes::SHA512SUM); + hashes.AddFD(fd); + HashStringList list = hashes.GetHashStringList(); + EXPECT_FALSE(list.empty()); + EXPECT_EQ(3, list.size()); + EXPECT_EQ(md5.Value(), list.find("MD5Sum")->HashValue()); + EXPECT_EQ(NULL, list.find("SHA1")); + EXPECT_EQ(NULL, list.find("SHA256")); + EXPECT_EQ(sha512.Value(), list.find("SHA512")->HashValue()); + EXPECT_EQ(FileSize, list.find("Checksum-FileSize")->HashValue()); + fd.Seek(0); + Hashes hashes2(list); + hashes2.AddFD(fd); + list = hashes2.GetHashStringList(); + EXPECT_FALSE(list.empty()); + EXPECT_EQ(3, list.size()); + EXPECT_EQ(md5.Value(), list.find("MD5Sum")->HashValue()); + EXPECT_EQ(NULL, list.find("SHA1")); + EXPECT_EQ(NULL, list.find("SHA256")); + EXPECT_EQ(sha512.Value(), list.find("SHA512")->HashValue()); + EXPECT_EQ(FileSize, list.find("Checksum-FileSize")->HashValue()); + } + fd.Seek(0); { MD5Summation MD5; MD5.AddFD(fd.Fd()); -- cgit v1.2.3-70-g09d2 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 ++- test/integration/test-apt-download-progress | 6 +++-- 7 files changed, 47 insertions(+), 24 deletions(-) (limited to 'methods/http.cc') 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; diff --git a/test/integration/test-apt-download-progress b/test/integration/test-apt-download-progress index b2c9effe6..07c5e09c5 100755 --- a/test/integration/test-apt-download-progress +++ b/test/integration/test-apt-download-progress @@ -26,14 +26,16 @@ assertprogress() { TESTFILE=testfile.big testsuccess dd if=/dev/zero of=./aptarchive/$TESTFILE bs=800k count=1 +OPT='-o APT::Status-Fd=3 -o Debug::pkgAcquire::Worker=1 -o Debug::Acquire::http=1 -o Debug::Acquire::https=1' + msgtest 'download progress works via' 'http' exec 3> apt-progress.log -testsuccess --nomsg apthelper download-file "http://localhost:8080/$TESTFILE" http-$TESTFILE -o APT::Status-Fd=3 -o Acquire::http::Dl-Limit=800 +testsuccess --nomsg apthelper download-file "http://localhost:8080/$TESTFILE" http-$TESTFILE $OPT -o Acquire::http::Dl-Limit=800 assertprogress apt-progress.log msgtest 'download progress works via' 'https' exec 3> apt-progress.log -testsuccess --nomsg apthelper download-file "https://localhost:4433/$TESTFILE" https-$TESTFILE -o APT::Status-Fd=3 -o Acquire::https::Dl-Limit=800 +testsuccess --nomsg apthelper download-file "https://localhost:4433/$TESTFILE" https-$TESTFILE $OPT -o Acquire::https::Dl-Limit=800 assertprogress apt-progress.log # cleanup -- cgit v1.2.3-70-g09d2 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 ++- test/interactive-helper/aptwebserver.cc | 6 +++--- 4 files changed, 21 insertions(+), 10 deletions(-) (limited to 'methods/http.cc') 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; diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc index cd52da692..9c67b67e4 100644 --- a/test/interactive-helper/aptwebserver.cc +++ b/test/interactive-helper/aptwebserver.cc @@ -654,13 +654,13 @@ static void * handleClient(void * voidclient) /*{{{*/ if (filesize > filestart) { data.Skip(filestart); - std::ostringstream contentlength; - contentlength << "Content-Length: " << (filesize - filestart); - headers.push_back(contentlength.str()); std::ostringstream contentrange; contentrange << "Content-Range: bytes " << filestart << "-" << filesize - 1 << "/" << filesize; headers.push_back(contentrange.str()); + std::ostringstream contentlength; + contentlength << "Content-Length: " << (filesize - filestart); + headers.push_back(contentlength.str()); sendHead(client, 206, headers); if (sendContent == true) sendFile(client, headers, data); -- cgit v1.2.3-70-g09d2