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 --- test/integration/test-partial-file-support | 62 +++++++++++++++++++++++++++--- 1 file changed, 56 insertions(+), 6 deletions(-) (limited to 'test/integration/test-partial-file-support') diff --git a/test/integration/test-partial-file-support b/test/integration/test-partial-file-support index 98b2f242a..b6b305d25 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='./downloaded/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' @@ -66,9 +104,11 @@ testrun() { testdownloadfile 'invalid partial data' "${1}/testfile" "$DOWN" '!=' testwebserverlaststatuscode '206' "$DOWNLOADLOG" - copysource $TESTFILE 1M $DOWN - testdownloadfile 'completely downloaded file' "${1}/testfile" "$DOWN" '=' - testwebserverlaststatuscode '416' "$DOWNLOADLOG" + webserverconfig 'aptwebserver::closeOnError' 'false' + followuprequest "$1" + webserverconfig 'aptwebserver::closeOnError' 'true' + followuprequest "$1" + webserverconfig 'aptwebserver::closeOnError' 'false' copysource /dev/zero 1M $DOWN testdownloadfile 'too-big partial file' "${1}/testfile" "$DOWN" '=' @@ -86,8 +126,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' -- cgit v1.2.3-70-g09d2 From 146f7715a9f36d246b461255b3c683b479296915 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Mon, 27 Apr 2015 10:59:27 +0200 Subject: improve partial/ cleanup in abort and failure cases Especially pdiff-enhanced downloads have the tendency to fail for various reasons from which we can recover and even a successful download used to leave the old unpatched index in partial/. By adding a new method responsible for making the transaction of an individual file happen we can at specialisations especially for abort cases to deal with the cleanup. This also helps in keeping the compressed indexes around if another index failed instead of keeping the decompressed files, which we wouldn't pick up in the next call. --- apt-pkg/acquire-item.cc | 193 +++++++++++++++++-------- apt-pkg/acquire-item.h | 17 ++- test/integration/framework | 6 +- test/integration/test-apt-update-expected-size | 2 +- test/integration/test-partial-file-support | 23 ++- test/integration/test-pdiff-usage | 2 +- 6 files changed, 161 insertions(+), 82 deletions(-) (limited to 'test/integration/test-partial-file-support') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 00862fe23..01ce0e650 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -173,6 +173,39 @@ void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf) ReportMirrorFailure(ErrorText); } /*}}}*/ +bool pkgAcquire::Item::TransactionState(TransactionStates const state) /*{{{*/ +{ + bool const Debug = _config->FindB("Debug::Acquire::Transaction", false); + switch(state) + { + case TransactionAbort: + if(Debug == true) + std::clog << " Cancel: " << DestFile << std::endl; + if (Status == pkgAcquire::Item::StatIdle) + { + Status = pkgAcquire::Item::StatDone; + Dequeue(); + } + break; + case TransactionCommit: + if(PartialFile != "") + { + if(Debug == true) + std::clog << "mv " << PartialFile << " -> "<< DestFile << " # " << DescURI() << std::endl; + + Rename(PartialFile, DestFile); + } else { + if(Debug == true) + std::clog << "rm " << DestFile << " # " << DescURI() << std::endl; + unlink(DestFile.c_str()); + } + // mark that this transaction is finished + TransactionManager = 0; + break; + } + return true; +} + /*}}}*/ // Acquire::Item::Start - Item has begun to download /*{{{*/ // --------------------------------------------------------------------- /* Stash status and the file size. Note that setting Complete means @@ -300,6 +333,9 @@ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const // the method is expected to report a good error for this Status = StatError; break; + case PDiffError: + // no handling here, done by callers + break; } return false; } @@ -374,7 +410,7 @@ pkgAcqDiffIndex::pkgAcqDiffIndex(pkgAcquire *Owner, HashStringList const &ExpectedHashes, indexRecords *MetaIndexParser) : pkgAcqBaseIndex(Owner, TransactionManager, Target, ExpectedHashes, - MetaIndexParser), PackagesFileReadyInPartial(false) + MetaIndexParser) { Debug = _config->FindB("Debug::pkgAcquire::Diffs",false); @@ -671,20 +707,6 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string IndexDiffFile) /*{{{*/ return false; } - // FIXME: make this use the method - PackagesFileReadyInPartial = true; - std::string const Partial = GetPartialFileNameFromURI(RealURI); - - FileFd From(CurrentPackagesFile, FileFd::ReadOnly); - FileFd To(Partial, FileFd::WriteEmpty); - if(CopyFile(From, To) == false) - return _error->Errno("CopyFile", "failed to copy"); - - if(Debug) - std::cerr << "Done copying " << CurrentPackagesFile - << " -> " << Partial - << std::endl; - // we have something, queue the diffs string::size_type const last_space = Description.rfind(" "); if(last_space != string::npos) @@ -738,6 +760,24 @@ void pkgAcqDiffIndex::Failed(string Message,pkgAcquire::MethodConfig * Cnf)/*{{{ new pkgAcqIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser); } /*}}}*/ +bool pkgAcqDiffIndex::TransactionState(TransactionStates const state) /*{{{*/ +{ + if (pkgAcquire::Item::TransactionState(state) == false) + return false; + + switch (state) + { + case TransactionCommit: + break; + case TransactionAbort: + std::string const Partial = GetPartialFileNameFromURI(RealURI); + unlink(Partial.c_str()); + break; + } + + return true; +} + /*}}}*/ void pkgAcqDiffIndex::Done(string Message,unsigned long long Size,HashStringList const &Hashes, /*{{{*/ pkgAcquire::MethodConfig *Cnf) { @@ -765,15 +805,21 @@ void pkgAcqDiffIndex::Done(string Message,unsigned long long Size,HashStringList if(StringToBool(LookupTag(Message,"IMS-Hit"),false)) DestFile = FinalFile; - if(!ParseDiffIndex(DestFile)) - return Failed("Message: Couldn't parse pdiff index", Cnf); + if(ParseDiffIndex(DestFile) == false) + { + Failed("Message: Couldn't parse pdiff index", Cnf); + // queue for final move - this should happen even if we fail + // while parsing (e.g. on sizelimit) and download the complete file. + TransactionManager->TransactionStageCopy(this, DestFile, FinalFile); + return; + } - // queue for final move TransactionManager->TransactionStageCopy(this, DestFile, FinalFile); Complete = true; Status = StatDone; Dequeue(); + return; } /*}}}*/ @@ -808,6 +854,17 @@ pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *Owner, } else { + // patching needs to be bootstrapped with the 'old' version + std::string const PartialFile = GetPartialFileNameFromURI(RealURI); + if (RealFileExists(PartialFile) == false) + { + if (symlink(GetFinalFilename().c_str(), PartialFile.c_str()) != 0) + { + Failed("Link creation of " + PartialFile + " to " + GetFinalFilename() + " failed", NULL); + return; + } + } + // get the next diff State = StateFetchDiff; QueueNextDiff(); @@ -822,6 +879,8 @@ void pkgAcqIndexDiffs::Failed(string Message,pkgAcquire::MethodConfig * Cnf)/*{{ if(Debug) std::clog << "pkgAcqIndexDiffs failed: " << Desc.URI << " with " << Message << std::endl << "Falling back to normal index file acquire" << std::endl; + DestFile = GetPartialFileNameFromURI(Target->URI); + RenameOnError(PDiffError); new pkgAcqIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser); Finish(); } @@ -950,6 +1009,8 @@ void pkgAcqIndexDiffs::Done(string Message,unsigned long long Size, HashStringLi if (fd.Size() != available_patches[0].patch_size || available_patches[0].patch_hashes != LocalHashes) { + // patchfiles are dated, so bad indicates a bad download, so kill it + unlink(DestFile.c_str()); Failed("Patch has Size/Hashsum mismatch", NULL); return; } @@ -1046,6 +1107,8 @@ void pkgAcqIndexMergeDiffs::Failed(string Message,pkgAcquire::MethodConfig * Cnf State = StateErrorDiff; if (Debug) std::clog << "Falling back to normal index file acquire" << std::endl; + DestFile = GetPartialFileNameFromURI(Target->URI); + RenameOnError(PDiffError); new pkgAcqIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser); } /*}}}*/ @@ -1069,6 +1132,8 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri if (fd.Size() != patch.patch_size || patch.patch_hashes != LocalHashes) { + // patchfiles are dated, so bad indicates a bad download, so kill it + unlink(DestFile.c_str()); Failed("Patch has Size/Hashsum mismatch", NULL); return; } @@ -1090,6 +1155,13 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri // this is the last completed diff, so we are ready to apply now State = StateApplyDiff; + // patching needs to be bootstrapped with the 'old' version + if (symlink(GetFinalFilename().c_str(), FinalFile.c_str()) != 0) + { + Failed("Link creation of " + FinalFile + " to " + GetFinalFilename() + " failed", NULL); + return; + } + if(Debug) std::clog << "Sending to rred method: " << FinalFile << std::endl; @@ -1109,15 +1181,14 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri return; } - // move the result into place - std::string const FinalFile = GetFinalFilename(); + std::string const Final = GetFinalFilename(); if(Debug) std::clog << "Queue patched file in place: " << std::endl - << DestFile << " -> " << FinalFile << std::endl; + << DestFile << " -> " << Final << std::endl; // queue for copy by the transaction manager - TransactionManager->TransactionStageCopy(this, DestFile, FinalFile); + TransactionManager->TransactionStageCopy(this, DestFile, Final); // ensure the ed's are gone regardless of list-cleanup for (std::vector::const_iterator I = allPatches->begin(); @@ -1127,6 +1198,7 @@ void pkgAcqIndexMergeDiffs::Done(string Message,unsigned long long Size,HashStri std::string patch = PartialFile + ".ed." + (*I)->patch.file + ".gz"; unlink(patch.c_str()); } + unlink(FinalFile.c_str()); // all set and done Complete = true; @@ -1337,12 +1409,6 @@ void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf) return; } - // on decompression failure, remove bad versions in partial/ - if (Stage == STAGE_DECOMPRESS_AND_VERIFY) - { - unlink(EraseFileName.c_str()); - } - Item::Failed(Message,Cnf); if(Target->IsOptional() && ExpectedHashes.empty() && Stage == STAGE_DOWNLOAD) @@ -1351,6 +1417,30 @@ void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf) TransactionManager->AbortTransaction(); } /*}}}*/ +bool pkgAcqIndex::TransactionState(TransactionStates const state) /*{{{*/ +{ + if (pkgAcquire::Item::TransactionState(state) == false) + return false; + + switch (state) + { + case TransactionAbort: + if (Stage == STAGE_DECOMPRESS_AND_VERIFY) + { + // keep the compressed file, but drop the decompressed + EraseFileName.clear(); + if (PartialFile.empty() == false && flExtension(PartialFile) == "decomp") + unlink(PartialFile.c_str()); + } + break; + case TransactionCommit: + if (EraseFileName.empty() == false) + unlink(EraseFileName.c_str()); + break; + } + return true; +} + /*}}}*/ // pkgAcqIndex::GetFinalFilename - Return the full final file path /*{{{*/ std::string pkgAcqIndex::GetFinalFilename() const { @@ -1530,9 +1620,6 @@ void pkgAcqIndex::StageDecompressDone(string Message, return; } - // remove the compressed version of the file - unlink(EraseFileName.c_str()); - // Done, queue for rename on transaction finished TransactionManager->TransactionStageCopy(this, DestFile, GetFinalFilename()); @@ -1568,14 +1655,7 @@ void pkgAcqMetaBase::AbortTransaction() for (std::vector::iterator I = Transaction.begin(); I != Transaction.end(); ++I) { - if(_config->FindB("Debug::Acquire::Transaction", false) == true) - std::clog << " Cancel: " << (*I)->DestFile << std::endl; - // the transaction will abort, so stop anything that is idle - if ((*I)->Status == pkgAcquire::Item::StatIdle) - { - (*I)->Status = pkgAcquire::Item::StatDone; - (*I)->Dequeue(); - } + (*I)->TransactionState(TransactionAbort); } Transaction.clear(); } @@ -1585,10 +1665,16 @@ bool pkgAcqMetaBase::TransactionHasError() { for (pkgAcquire::ItemIterator I = Transaction.begin(); I != Transaction.end(); ++I) - if((*I)->Status != pkgAcquire::Item::StatDone && - (*I)->Status != pkgAcquire::Item::StatIdle) - return true; - + { + switch((*I)->Status) { + case StatDone: break; + case StatIdle: break; + case StatAuthError: return true; + case StatError: return true; + case StatTransientNetworkError: return true; + case StatFetching: break; + } + } return false; } /*}}}*/ @@ -1603,24 +1689,7 @@ void pkgAcqMetaBase::CommitTransaction() for (std::vector::iterator I = Transaction.begin(); I != Transaction.end(); ++I) { - if((*I)->PartialFile != "") - { - if(_config->FindB("Debug::Acquire::Transaction", false) == true) - std::clog << "mv " << (*I)->PartialFile << " -> "<< (*I)->DestFile << " " - << (*I)->DescURI() << std::endl; - - Rename((*I)->PartialFile, (*I)->DestFile); - } else { - if(_config->FindB("Debug::Acquire::Transaction", false) == true) - std::clog << "rm " - << (*I)->DestFile - << " " - << (*I)->DescURI() - << std::endl; - unlink((*I)->DestFile.c_str()); - } - // mark that this transaction is finished - (*I)->TransactionManager = 0; + (*I)->TransactionState(TransactionCommit); } Transaction.clear(); } @@ -1634,7 +1703,7 @@ void pkgAcqMetaBase::TransactionStageCopy(Item *I, I->DestFile = To; } /*}}}*/ -// AcqMetaBase::TransactionStageRemoval - Sage a file for removal /*{{{*/ +// AcqMetaBase::TransactionStageRemoval - Stage a file for removal /*{{{*/ void pkgAcqMetaBase::TransactionStageRemoval(Item *I, const std::string &FinalFile) { diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 148728b3c..33a28671c 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -344,7 +344,8 @@ class pkgAcquire::Item : public WeakPointable InvalidFormat, SignatureError, NotClearsigned, - MaximumSizeExceeded + MaximumSizeExceeded, + PDiffError, }; /** \brief Rename failed file and set error @@ -353,6 +354,12 @@ class pkgAcquire::Item : public WeakPointable */ bool RenameOnError(RenameOnErrorState const state); + enum TransactionStates { + TransactionCommit, + TransactionAbort, + }; + virtual bool TransactionState(TransactionStates const state); + /** \brief The HashSums of the item is supposed to have than done */ HashStringList ExpectedHashes; @@ -685,14 +692,12 @@ class APT_HIDDEN pkgAcqDiffIndex : public pkgAcqBaseIndex */ std::string Description; - /** \brief If the copy step of the packages file is done - */ - bool PackagesFileReadyInPartial; - /** \brief Get the full pathname of the final file for the current URI */ virtual std::string GetFinalFilename() const; virtual bool QueueURI(pkgAcquire::ItemDesc &Item); + + virtual bool TransactionState(TransactionStates const state); public: // Specialized action members virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf); @@ -1010,6 +1015,8 @@ class APT_HIDDEN pkgAcqIndex : public pkgAcqBaseIndex /** \brief Get the full pathname of the final file for the current URI */ virtual std::string GetFinalFilename() const; + virtual bool TransactionState(TransactionStates const state); + public: // Specialized action members virtual void Failed(std::string Message,pkgAcquire::MethodConfig *Cnf); diff --git a/test/integration/framework b/test/integration/framework index 4229ae162..7564a0faf 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1523,9 +1523,13 @@ aptautotest_aptget_update() { testfilestats "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:755" testfilestats "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:755" # all copied files are properly chmodded - for file in $(find "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" -maxdepth 1 -type f ! -name 'lock'); do + for file in $(find "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists" -type f ! -name 'lock'); do testfilestats "$file" '%U:%G:%a' '=' "${TEST_DEFAULT_USER}:${TEST_DEFAULT_GROUP}:644" done + if [ "$1" = 'testsuccess' ]; then + # failure cases can retain partial files and such + testempty find "${TMPWORKINGDIRECTORY}/rootdir/var/lib/apt/lists/partial" -mindepth 1 ! \( -name 'lock' -o -name '*.FAILED' \) + fi } aptautotest_apt_update() { aptautotest_aptget_update "$@"; } aptautotest_aptcdrom_add() { aptautotest_aptget_update "$@"; } diff --git a/test/integration/test-apt-update-expected-size b/test/integration/test-apt-update-expected-size index 9f58308aa..7efccaa57 100755 --- a/test/integration/test-apt-update-expected-size +++ b/test/integration/test-apt-update-expected-size @@ -34,7 +34,7 @@ test_packagestoobig() { done NEW_SIZE="$(stat --printf=%s aptarchive/dists/unstable/main/binary-i386/Packages)" testfailuremsg "W: Failed to fetch ${1}/dists/unstable/main/binary-i386/Packages Writing more data than expected ($NEW_SIZE > $SIZE) -E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -o Debug::pkgAcquire::Worker=0 +E: Some index files failed to download. They have been ignored, or old ones used instead." aptget update -o Debug::pkgAcquire::Worker=0 -o Debug::Acquire::Transaction=0 } methodtest() { diff --git a/test/integration/test-partial-file-support b/test/integration/test-partial-file-support index b6b305d25..85046b3eb 100755 --- a/test/integration/test-partial-file-support +++ b/test/integration/test-partial-file-support @@ -126,18 +126,17 @@ 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' +serverconfigs() { + msgmsg "${1%%:*}: Test with Content-Length" + webserverconfig 'aptwebserver::chunked-transfer-encoding' 'false' + testrun "$1" + msgmsg "${1%%:*}: Test with Transfer-Encoding: chunked" + webserverconfig 'aptwebserver::chunked-transfer-encoding' 'true' + testrun "$1" +} + +serverconfigs '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' +serverconfigs 'https://localhost:4433' diff --git a/test/integration/test-pdiff-usage b/test/integration/test-pdiff-usage index 5e759e50e..7d72a6944 100755 --- a/test/integration/test-pdiff-usage +++ b/test/integration/test-pdiff-usage @@ -47,7 +47,7 @@ testrun() { compressfile 'aptarchive/Packages' generatereleasefiles signreleasefiles - rm -rf aptarchive/Packages.diff rootdir/var/lib/apt/lists + rm -rf aptarchive/Packages.diff rootdir/var/lib/apt/lists rootdir/var/lib/apt/lists-bak testsuccess aptget update "$@" cp -a rootdir/var/lib/apt/lists rootdir/var/lib/apt/lists-bak testnopackage newstuff -- cgit v1.2.3-70-g09d2 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 +++++++++++--- test/integration/framework | 2 +- test/integration/test-apt-update-transactions | 1 + test/integration/test-partial-file-support | 10 ++++++-- test/interactive-helper/aptwebserver.cc | 9 ++++--- 7 files changed, 62 insertions(+), 17 deletions(-) (limited to 'test/integration/test-partial-file-support') 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); diff --git a/test/integration/framework b/test/integration/framework index 03c188189..2a53e8365 100644 --- a/test/integration/framework +++ b/test/integration/framework @@ -1128,7 +1128,7 @@ acquire::cdrom::autodetect 0;" > rootdir/etc/apt/apt.conf.d/00cdrom downloadfile() { local PROTO="${1%%:*}" if ! apthelper -o Debug::Acquire::${PROTO}=1 -o Debug::pkgAcquire::Worker=1 \ - download-file "$1" "$2" 2>&1 ; then + download-file "$1" "$2" "$3" 2>&1 ; then return 1 fi # only if the file exists the download was successful diff --git a/test/integration/test-apt-update-transactions b/test/integration/test-apt-update-transactions index f028ac0c7..67dd633f9 100755 --- a/test/integration/test-apt-update-transactions +++ b/test/integration/test-apt-update-transactions @@ -63,6 +63,7 @@ testsetup 'file' changetowebserver webserverconfig 'aptwebserver::support::modified-since' 'false' "$1" webserverconfig 'aptwebserver::support::last-modified' 'false' "$1" # curl is clever and sees hits here also +webserverconfig 'aptwebserver::support::range' 'false' "$1" testsetup 'http' diff --git a/test/integration/test-partial-file-support b/test/integration/test-partial-file-support index 85046b3eb..c07af7bd0 100755 --- a/test/integration/test-partial-file-support +++ b/test/integration/test-partial-file-support @@ -17,8 +17,8 @@ DOWNLOADLOG='rootdir/tmp/testdownloadfile.log' testdownloadfile() { rm -f "$DOWNLOADLOG" - msgtest "Testing download of file $2 with" "$1" - if ! downloadfile "$2" "$3" > "$DOWNLOADLOG"; then + msgtest "Testing download of file $2 with" "$1 $5" + if ! downloadfile "$2" "$3" "$5" > "$DOWNLOADLOG"; then cat >&2 "$DOWNLOADLOG" msgfail else @@ -78,6 +78,12 @@ followuprequest() { testdownloadfile 'completely downloaded file' "${1}/testfile" "$DOWN" '=' testwebserverlaststatuscode '416' "$DOWNLOADLOG" + webserverconfig 'aptwebserver::support::content-range' 'false' + copysource $TESTFILE 1M $DOWN + testdownloadfile 'completely downloaded file' "${1}/testfile" "$DOWN" '=' "SHA1:$(sha1sum "$TESTFILE" | cut -d' ' -f 1)" + testwebserverlaststatuscode '416' "$DOWNLOADLOG" + webserverconfig 'aptwebserver::support::content-range' 'true' + copysource $TESTFILE 1M $DOWN copysource "${TESTFILE}2" 20 "${DOWN}2" msgtest 'Testing download of files with' 'completely downloaded file + partial file' diff --git a/test/interactive-helper/aptwebserver.cc b/test/interactive-helper/aptwebserver.cc index 6a411e24e..c933060e7 100644 --- a/test/interactive-helper/aptwebserver.cc +++ b/test/interactive-helper/aptwebserver.cc @@ -745,9 +745,12 @@ static void * handleClient(void * voidclient) /*{{{*/ } else { - std::ostringstream contentrange; - contentrange << "Content-Range: bytes */" << filesize; - headers.push_back(contentrange.str()); + if (_config->FindB("aptwebserver::support::content-range", true) == true) + { + std::ostringstream contentrange; + contentrange << "Content-Range: bytes */" << filesize; + headers.push_back(contentrange.str()); + } sendError(client, 416, *m, sendContent, "", headers); break; } -- cgit v1.2.3-70-g09d2