From 03aa08472dcd689572a46ce6efdb1dccf6136334 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 23 Oct 2014 01:28:05 +0200 Subject: chown finished partial files earlier partial files are chowned by the Item baseclass to let the methods work with them. Now, this baseclass is also responsible for chowning the files back to root instead of having various deeper levels do this. The consequence is that all overloaded Failed() methods now call the Item::Failed base as their first step. The same is done for Done(). The effect is that even in partial files usually don't belong to _apt anymore, helping sneakernets and reducing possibilities of a bad method modifying files not belonging to them. The change is supported by the framework not only supporting being run as root, but with proper permission management, too, so that privilege dropping can be tested with them. --- apt-pkg/acquire-item.cc | 111 ++++++++++++++++++++++++++---------------------- apt-pkg/acquire-item.h | 1 + apt-pkg/acquire.cc | 40 ++++++++++------- apt-pkg/acquire.h | 2 + 4 files changed, 89 insertions(+), 65 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 704e285b5..11c522ae5 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -67,11 +67,11 @@ static void printHashSumComparision(std::string const &URI, HashStringList const /*}}}*/ static void ChangeOwnerAndPermissionOfFile(char const * const requester, char const * const file, char const * const user, char const * const group, mode_t const mode) /*{{{*/ { - // ensure the file is owned by root and has good permissions - struct passwd const * const pw = getpwnam(user); - struct group const * const gr = getgrnam(group); - if (getuid() == 0) // if we aren't root, we can't chown, so don't try it + if (getuid() == 0 && strlen(user) != 0 && strlen(group) != 0) // if we aren't root, we can't chown, so don't try it { + // ensure the file is owned by root and has good permissions + struct passwd const * const pw = getpwnam(user); + struct group const * const gr = getgrnam(group); if (pw != NULL && gr != NULL && chown(file, pw->pw_uid, gr->gr_gid) != 0) _error->WarningE(requester, "chown to %s:%s of file %s failed", user, group, file); } @@ -155,7 +155,10 @@ pkgAcquire::Item::~Item() fetch this object */ void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf) { - if(ErrorText == "") + if (RealFileExists(DestFile)) + ChangeOwnerAndPermissionOfFile("Item::Failed", DestFile.c_str(), "root", "root", 0644); + + if(ErrorText.empty()) ErrorText = LookupTag(Message,"Message"); UsedMirror = LookupTag(Message,"UsedMirror"); if (QueueCounter <= 1) @@ -179,9 +182,9 @@ void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf) Status = StatIdle; // check fail reason - string FailReason = LookupTag(Message, "FailReason"); + string const FailReason = LookupTag(Message, "FailReason"); if(FailReason == "MaximumSizeExceeded") - Rename(DestFile, DestFile+".FAILED"); + RenameOnError(MaximumSizeExceeded); // report mirror failure back to LP if we actually use a mirror if(FailReason.size() != 0) @@ -197,6 +200,7 @@ void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf) void pkgAcquire::Item::Start(string /*Message*/,unsigned long long Size) { Status = StatFetching; + ErrorText.clear(); if (FileSize == 0 && Complete == false) FileSize = Size; } @@ -215,6 +219,8 @@ void pkgAcquire::Item::Done(string Message,unsigned long long Size,HashStringLis if (Owner->Log != 0) Owner->Log->Fetched(Size,atoi(LookupTag(Message,"Resume-Point","0").c_str())); } + if (RealFileExists(DestFile)) + ChangeOwnerAndPermissionOfFile("Item::Done", DestFile.c_str(), "root", "root", 0644); if (FileSize == 0) FileSize= Size; @@ -231,6 +237,7 @@ bool pkgAcquire::Item::Rename(string From,string To) { if (rename(From.c_str(),To.c_str()) == 0) return true; + ChangeOwnerAndPermissionOfFile("Item::Failed", To.c_str(), "root", "root", 0644); std::string S; strprintf(S, _("rename failed, %s (%s -> %s)."), strerror(errno), @@ -258,7 +265,7 @@ void pkgAcquire::Item::Dequeue() /*{{{*/ /*}}}*/ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const error)/*{{{*/ { - if(FileExists(DestFile)) + if (RealFileExists(DestFile)) Rename(DestFile, DestFile + ".FAILED"); switch (error) @@ -283,7 +290,11 @@ bool pkgAcquire::Item::RenameOnError(pkgAcquire::Item::RenameOnErrorState const Status = StatError; break; case NotClearsigned: - ErrorText = _("Does not start with a cleartext signature"); + ErrorText = _("Does not start with a cleartext signature"); + Status = StatError; + break; + case MaximumSizeExceeded: + // the method is expected to report a good error for this Status = StatError; break; } @@ -702,14 +713,14 @@ bool pkgAcqDiffIndex::ParseDiffIndex(string IndexDiffFile) /*{{{*/ /*}}}*/ void pkgAcqDiffIndex::Failed(string Message,pkgAcquire::MethodConfig * Cnf)/*{{{*/ { + Item::Failed(Message,Cnf); + Status = StatDone; + if(Debug) std::clog << "pkgAcqDiffIndex failed: " << Desc.URI << " with " << Message << std::endl << "Falling back to normal index file acquire" << std::endl; new pkgAcqIndex(Owner, TransactionManager, Target, ExpectedHashes, MetaIndexParser); - - Item::Failed(Message,Cnf); - Status = StatDone; } /*}}}*/ void pkgAcqDiffIndex::Done(string Message,unsigned long long Size,HashStringList const &Hashes, /*{{{*/ @@ -792,8 +803,11 @@ pkgAcqIndexDiffs::pkgAcqIndexDiffs(pkgAcquire *Owner, } } /*}}}*/ -void pkgAcqIndexDiffs::Failed(string Message,pkgAcquire::MethodConfig * /*Cnf*/)/*{{{*/ +void pkgAcqIndexDiffs::Failed(string Message,pkgAcquire::MethodConfig * Cnf)/*{{{*/ { + Item::Failed(Message,Cnf); + Status = StatDone; + if(Debug) std::clog << "pkgAcqIndexDiffs failed: " << Desc.URI << " with " << Message << std::endl << "Falling back to normal index file acquire" << std::endl; @@ -1281,11 +1295,14 @@ string pkgAcqIndex::Custom600Headers() const // pkgAcqIndex::Failed - getting the indexfile failed /*{{{*/ void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf) { + Item::Failed(Message,Cnf); + size_t const nextExt = CompressionExtensions.find(' '); if (nextExt != std::string::npos) { CompressionExtensions = CompressionExtensions.substr(nextExt+1); Init(RealURI, Desc.Description, Desc.ShortDesc); + Status = StatIdle; return; } @@ -1295,8 +1312,6 @@ void pkgAcqIndex::Failed(string Message,pkgAcquire::MethodConfig *Cnf) unlink(EraseFileName.c_str()); } - Item::Failed(Message,Cnf); - /// cancel the entire transaction TransactionManager->AbortTransaction(); } @@ -1521,6 +1536,8 @@ string pkgAcqIndexTrans::Custom600Headers() const // AcqIndexTrans::Failed - Silence failure messages for missing files /*{{{*/ void pkgAcqIndexTrans::Failed(string Message,pkgAcquire::MethodConfig *Cnf) { + Item::Failed(Message,Cnf); + size_t const nextExt = CompressionExtensions.find(' '); if (nextExt != std::string::npos) { @@ -1530,8 +1547,6 @@ void pkgAcqIndexTrans::Failed(string Message,pkgAcquire::MethodConfig *Cnf) return; } - Item::Failed(Message,Cnf); - // FIXME: this is used often (e.g. in pkgAcqIndexTrans) so refactor if (Cnf->LocalOnly == true || StringToBool(LookupTag(Message,"Transient-Failure"),false) == false) @@ -1563,16 +1578,9 @@ void pkgAcqMetaBase::AbortTransaction() if ((*I)->Status == pkgAcquire::Item::StatIdle) (*I)->Status = pkgAcquire::Item::StatDone; - // kill failed files in partial - if ((*I)->Status == pkgAcquire::Item::StatError) - { - std::string const PartialFile = GetPartialFileName(flNotDir((*I)->DestFile)); - if(FileExists(PartialFile)) - Rename(PartialFile, PartialFile + ".FAILED"); - } - // fix permissions for existing files which were part of a reverify - // like InRelease files or files in partial we might work with next time - else if (FileExists((*I)->DestFile)) + // reverify might act on a file outside of partial + // (as it itself is good, but needed to verify others, like Release) + if ((*I)->DestFile == (*I)->PartialFile && RealFileExists((*I)->DestFile)) ChangeOwnerAndPermissionOfFile("AbortTransaction", (*I)->DestFile.c_str(), "root", "root", 0644); } Transaction.clear(); @@ -1608,8 +1616,6 @@ void pkgAcqMetaBase::CommitTransaction() << (*I)->DescURI() << std::endl; Rename((*I)->PartialFile, (*I)->DestFile); - ChangeOwnerAndPermissionOfFile("CommitTransaction", (*I)->DestFile.c_str(), "root", "root", 0644); - } else { if(_config->FindB("Debug::Acquire::Transaction", false) == true) std::clog << "rm " @@ -1758,16 +1764,17 @@ void pkgAcqMetaSig::Done(string Message,unsigned long long Size, /*}}}*/ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/ { - string Final = _config->FindDir("Dir::State::lists") + URItoFileName(RealURI); + Item::Failed(Message,Cnf); // check if we need to fail at this point if (AuthPass == true && CheckStopAuthentication(RealURI, Message)) return; // FIXME: meh, this is not really elegant - string InReleaseURI = RealURI.replace(RealURI.rfind("Release.gpg"), 12, + string const Final = _config->FindDir("Dir::State::lists") + URItoFileName(RealURI); + string const InReleaseURI = RealURI.replace(RealURI.rfind("Release.gpg"), 12, "InRelease"); - string FinalInRelease = _config->FindDir("Dir::State::lists") + URItoFileName(InReleaseURI); + string const FinalInRelease = _config->FindDir("Dir::State::lists") + URItoFileName(InReleaseURI); if (RealFileExists(Final) || RealFileExists(FinalInRelease)) { @@ -1782,7 +1789,7 @@ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/ _error->Warning(_("This is normally not allowed, but the option " "Acquire::AllowDowngradeToInsecureRepositories was " "given to override it.")); - + Status = StatDone; } else { _error->Error("%s", downgrade_msg.c_str()); Rename(MetaIndexFile, MetaIndexFile+".FAILED"); @@ -1810,8 +1817,6 @@ void pkgAcqMetaSig::Failed(string Message,pkgAcquire::MethodConfig *Cnf)/*{{{*/ QueueIndexes(true); } - Item::Failed(Message,Cnf); - // FIXME: this is used often (e.g. in pkgAcqIndexTrans) so refactor if (Cnf->LocalOnly == true || StringToBool(LookupTag(Message,"Transient-Failure"),false) == false) @@ -2226,10 +2231,12 @@ string pkgAcqMetaClearSig::Custom600Headers() const /*}}}*/ // pkgAcqMetaClearSig::Done - We got a file /*{{{*/ // --------------------------------------------------------------------- -void pkgAcqMetaClearSig::Done(std::string Message,unsigned long long /*Size*/, - HashStringList const &/*Hashes*/, +void pkgAcqMetaClearSig::Done(std::string Message,unsigned long long Size, + HashStringList const &Hashes, pkgAcquire::MethodConfig *Cnf) { + Item::Done(Message, Size, Hashes, Cnf); + // if we expect a ClearTextSignature (InRelase), ensure that // this is what we get and if not fail to queue a // Release/Release.gpg, see #346386 @@ -2567,7 +2574,6 @@ void pkgAcqArchive::Done(string Message,unsigned long long Size, HashStringList string FinalFile = _config->FindDir("Dir::Cache::Archives"); FinalFile += flNotDir(StoreFilename); Rename(DestFile,FinalFile); - ChangeOwnerAndPermissionOfFile("pkgAcqArchive::Done", FinalFile.c_str(), "root", "root", 0644); StoreFilename = DestFile = FinalFile; Complete = true; } @@ -2577,8 +2583,8 @@ void pkgAcqArchive::Done(string Message,unsigned long long Size, HashStringList /* Here we try other sources */ void pkgAcqArchive::Failed(string Message,pkgAcquire::MethodConfig *Cnf) { - ErrorText = LookupTag(Message,"Message"); - + Item::Failed(Message,Cnf); + /* We don't really want to retry on failed media swaps, this prevents that. An interesting observation is that permanent failures are not recorded. */ @@ -2588,10 +2594,10 @@ void pkgAcqArchive::Failed(string Message,pkgAcquire::MethodConfig *Cnf) // Vf = Version.FileList(); while (Vf.end() == false) ++Vf; StoreFilename = string(); - Item::Failed(Message,Cnf); return; } - + + Status = StatIdle; if (QueueNext() == false) { // This is the retry counter @@ -2604,9 +2610,9 @@ void pkgAcqArchive::Failed(string Message,pkgAcquire::MethodConfig *Cnf) if (QueueNext() == true) return; } - + StoreFilename = string(); - Item::Failed(Message,Cnf); + Status = StatError; } } /*}}}*/ @@ -2726,7 +2732,12 @@ void pkgAcqFile::Done(string Message,unsigned long long Size,HashStringList cons // Symlink the file if (symlink(FileName.c_str(),DestFile.c_str()) != 0) { - ErrorText = "Link to " + DestFile + " failure "; + _error->PushToStack(); + _error->Errno("pkgAcqFile::Done", "Symlinking file %s failed", DestFile.c_str()); + std::stringstream msg; + _error->DumpErrors(msg); + _error->RevertToStack(); + ErrorText = msg.str(); Status = StatError; Complete = false; } @@ -2738,19 +2749,19 @@ void pkgAcqFile::Done(string Message,unsigned long long Size,HashStringList cons /* Here we try other sources */ void pkgAcqFile::Failed(string Message,pkgAcquire::MethodConfig *Cnf) { - ErrorText = LookupTag(Message,"Message"); - + Item::Failed(Message,Cnf); + // This is the retry counter if (Retries != 0 && Cnf->LocalOnly == false && StringToBool(LookupTag(Message,"Transient-Failure"),false) == true) { - Retries--; + --Retries; QueueURI(Desc); + Status = StatIdle; return; } - - Item::Failed(Message,Cnf); + } /*}}}*/ // AcqIndex::Custom600Headers - Insert custom request headers /*{{{*/ diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 68d5a01ce..4b4ef5fca 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -323,6 +323,7 @@ class pkgAcquire::Item : public WeakPointable InvalidFormat, SignatureError, NotClearsigned, + MaximumSizeExceeded }; /** \brief Rename failed file and set error diff --git a/apt-pkg/acquire.cc b/apt-pkg/acquire.cc index 1e20b74be..2c89c2dea 100644 --- a/apt-pkg/acquire.cc +++ b/apt-pkg/acquire.cc @@ -54,23 +54,38 @@ pkgAcquire::pkgAcquire() : LockFD(-1), Queues(0), Workers(0), Configs(0), Log(NU Debug(_config->FindB("Debug::pkgAcquire",false)), Running(false) { - string const Mode = _config->Find("Acquire::Queue-Mode","host"); - if (strcasecmp(Mode.c_str(),"host") == 0) - QueueMode = QueueHost; - if (strcasecmp(Mode.c_str(),"access") == 0) - QueueMode = QueueAccess; + Initialize(); } pkgAcquire::pkgAcquire(pkgAcquireStatus *Progress) : LockFD(-1), Queues(0), Workers(0), Configs(0), Log(NULL), ToFetch(0), Debug(_config->FindB("Debug::pkgAcquire",false)), Running(false) +{ + Initialize(); + SetLog(Progress); +} +void pkgAcquire::Initialize() { string const Mode = _config->Find("Acquire::Queue-Mode","host"); if (strcasecmp(Mode.c_str(),"host") == 0) QueueMode = QueueHost; if (strcasecmp(Mode.c_str(),"access") == 0) QueueMode = QueueAccess; - SetLog(Progress); + + // chown the auth.conf file as it will be accessed by our methods + std::string const SandboxUser = _config->Find("APT::Sandbox::User"); + if (getuid() == 0 && SandboxUser.empty() == false) // if we aren't root, we can't chown, so don't try it + { + struct passwd const * const pw = getpwnam(SandboxUser.c_str()); + struct group const * const gr = getgrnam("root"); + if (pw != NULL && gr != NULL) + { + std::string const AuthConf = _config->FindFile("Dir::Etc::netrc"); + if(AuthConf.empty() == false && RealFileExists(AuthConf) && + chown(AuthConf.c_str(), pw->pw_uid, gr->gr_gid) != 0) + _error->WarningE("SetupAPTPartialDirectory", "chown to %s:root of file %s failed", SandboxUser.c_str(), AuthConf.c_str()); + } + } } /*}}}*/ // Acquire::GetLock - lock directory and prepare for action /*{{{*/ @@ -81,21 +96,16 @@ static bool SetupAPTPartialDirectory(std::string const &grand, std::string const CreateAPTDirectoryIfNeeded(parent, partial) == false) return false; - if (getuid() == 0) // if we aren't root, we can't chown, so don't try it + std::string const SandboxUser = _config->Find("APT::Sandbox::User"); + if (getuid() == 0 && SandboxUser.empty() == false) // if we aren't root, we can't chown, so don't try it { - std::string SandboxUser = _config->Find("APT::Sandbox::User"); - struct passwd *pw = getpwnam(SandboxUser.c_str()); - struct group *gr = getgrnam("root"); + struct passwd const * const pw = getpwnam(SandboxUser.c_str()); + struct group const * const gr = getgrnam("root"); if (pw != NULL && gr != NULL) { // chown the partial dir if(chown(partial.c_str(), pw->pw_uid, gr->gr_gid) != 0) _error->WarningE("SetupAPTPartialDirectory", "chown to %s:root of directory %s failed", SandboxUser.c_str(), partial.c_str()); - // chown the auth.conf file - std::string const AuthConf = _config->FindFile("Dir::Etc::netrc"); - if(AuthConf.empty() == false && RealFileExists(AuthConf) && - chown(AuthConf.c_str(), pw->pw_uid, gr->gr_gid) != 0) - _error->WarningE("SetupAPTPartialDirectory", "chown to %s:root of file %s failed", SandboxUser.c_str(), AuthConf.c_str()); } } if (chmod(partial.c_str(), 0700) != 0) diff --git a/apt-pkg/acquire.h b/apt-pkg/acquire.h index a1a192d5f..f33362922 100644 --- a/apt-pkg/acquire.h +++ b/apt-pkg/acquire.h @@ -378,6 +378,8 @@ class pkgAcquire */ virtual ~pkgAcquire(); + private: + APT_HIDDEN void Initialize(); }; /** \brief Represents a single download source from which an item -- cgit v1.2.3 From 23397c9d7d4d455461176600bb45c81185493504 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Thu, 23 Oct 2014 16:54:00 +0200 Subject: promote filesize to a hashstring It is a very simple hashstring, which is why it isn't contributing to the usability of a list of them, but it is also trivial to check and calculate, so it doesn't hurt checking it either as it can combined even with the simplest other hashes greatly complicate attacks on them as you suddenly need a same-size hash collision, which is usually a lot harder to achieve. --- apt-pkg/contrib/hashes.cc | 32 +++++++++++++++++++++++++++----- apt-pkg/contrib/hashes.h | 4 ++-- apt-pkg/indexrecords.cc | 3 +++ 3 files changed, 32 insertions(+), 7 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/hashes.cc b/apt-pkg/contrib/hashes.cc index 417982343..55180c642 100644 --- a/apt-pkg/contrib/hashes.cc +++ b/apt-pkg/contrib/hashes.cc @@ -29,7 +29,7 @@ const char * HashString::_SupportedHashes[] = { - "SHA512", "SHA256", "SHA1", "MD5Sum", NULL + "SHA512", "SHA256", "SHA1", "MD5Sum", "Checksum-FileSize", NULL }; HashString::HashString() @@ -111,6 +111,8 @@ std::string HashString::GetHashForFile(std::string filename) const /*{{{*/ SHA512.AddFD(Fd); fileHash = (std::string)SHA512.Result(); } + else if (strcasecmp(Type.c_str(), "Checksum-FileSize") == 0) + strprintf(fileHash, "%llu", Fd.FileSize()); Fd.Close(); return fileHash; @@ -147,7 +149,13 @@ bool HashStringList::usable() const /*{{{*/ return false; std::string const forcedType = _config->Find("Acquire::ForceHash", ""); if (forcedType.empty() == true) - return true; + { + // FileSize alone isn't usable + for (std::vector::const_iterator hs = list.begin(); hs != list.end(); ++hs) + if (hs->HashType() != "Checksum-FileSize") + return true; + return false; + } return find(forcedType) != NULL; } /*}}}*/ @@ -201,6 +209,9 @@ bool HashStringList::VerifyFile(std::string filename) const /*{{{*/ HashString const * const hs = find(NULL); if (hs == NULL || hs->VerifyFile(filename) == false) return false; + HashString const * const hsf = find("Checksum-FileSize"); + if (hsf != NULL && hsf->VerifyFile(filename) == false) + return false; return true; } /*}}}*/ @@ -235,6 +246,14 @@ bool HashStringList::operator!=(HashStringList const &other) const } /*}}}*/ +// PrivateHashes /*{{{*/ +class PrivateHashes { +public: + unsigned long long FileSize; + + PrivateHashes() : FileSize(0) {} +}; + /*}}}*/ // 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) { @@ -254,6 +273,7 @@ bool Hashes::Add(const unsigned char * const Data,unsigned long long const Size, #if __GNUC__ >= 4 #pragma GCC diagnostic pop #endif + d->FileSize += Size; return Res; } bool Hashes::AddFD(int const Fd,unsigned long long Size, unsigned int const Hashes) @@ -314,15 +334,17 @@ HashStringList Hashes::GetHashStringList() #if __GNUC__ >= 4 #pragma GCC diagnostic pop #endif + std::string SizeStr; + strprintf(SizeStr, "%llu", d->FileSize); + hashes.push_back(HashString("Checksum-FileSize", SizeStr)); return hashes; } #if __GNUC__ >= 4 #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" - #pragma GCC diagnostic ignored "-Wsuggest-attribute=const" #endif -Hashes::Hashes() {} -Hashes::~Hashes() {} +Hashes::Hashes() { d = new PrivateHashes(); } +Hashes::~Hashes() { delete d; } #if __GNUC__ >= 4 #pragma GCC diagnostic pop #endif diff --git a/apt-pkg/contrib/hashes.h b/apt-pkg/contrib/hashes.h index caeba006d..e2e213855 100644 --- a/apt-pkg/contrib/hashes.h +++ b/apt-pkg/contrib/hashes.h @@ -161,10 +161,10 @@ class HashStringList std::vector list; }; +class PrivateHashes; class Hashes { - /** \brief dpointer placeholder */ - void *d; + PrivateHashes *d; public: /* those will disappear in the future as it is hard to add new ones this way. diff --git a/apt-pkg/indexrecords.cc b/apt-pkg/indexrecords.cc index bf1901e11..e1e9ba657 100644 --- a/apt-pkg/indexrecords.cc +++ b/apt-pkg/indexrecords.cc @@ -116,6 +116,9 @@ bool indexRecords::Load(const string Filename) /*{{{*/ indexRecords::checkSum *Sum = new indexRecords::checkSum; Sum->MetaKeyFilename = Name; Sum->Size = Size; + std::string SizeStr; + strprintf(SizeStr, "%llu", Size); + Sum->Hashes.push_back(HashString("Checksum-FileSize", SizeStr)); #if __GNUC__ >= 4 #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wdeprecated-declarations" -- cgit v1.2.3 From d8c71b3b5dc98daa247433503ad8242c9e7b77db Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Fri, 24 Oct 2014 23:55:15 +0200 Subject: rewrite ReadMessages() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Central methods of our infrastructure like this one responsible for communication with our methods shouldn't be more complicated then they have to and not claim to have (albeit unlikely) bugs. While I am not sure about having improved the first part, the bug is now gone and a few explicit tests check that it stays that way, so nobody will notice the difference (hopefully) – expect that this should a very tiny bit faster as well as we don't manually proceed through the string. Git-Dch: Ignore --- apt-pkg/contrib/strutl.cc | 124 ++++++++++++++++++++++++---------------------- 1 file changed, 66 insertions(+), 58 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/contrib/strutl.cc b/apt-pkg/contrib/strutl.cc index aad358a55..ebf9c9ea6 100644 --- a/apt-pkg/contrib/strutl.cc +++ b/apt-pkg/contrib/strutl.cc @@ -779,86 +779,94 @@ string TimeRFC1123(time_t Date) In particular: this reads blocks from the input until it believes that it's run out of input text. Each block is terminated by a - double newline ('\n' followed by '\n'). As noted below, there is a - bug in this code: it assumes that all the blocks have been read if - it doesn't see additional text in the buffer after the last one is - parsed, which will cause it to lose blocks if the last block - coincides with the end of the buffer. + double newline ('\n' followed by '\n'). */ bool ReadMessages(int Fd, vector &List) { char Buffer[64000]; - char *End = Buffer; // Represents any left-over from the previous iteration of the // parse loop. (i.e., if a message is split across the end // of the buffer, it goes here) string PartialMessage; - - while (1) - { - int Res = read(Fd,End,sizeof(Buffer) - (End-Buffer)); + + do { + int const Res = read(Fd, Buffer, sizeof(Buffer)); if (Res < 0 && errno == EINTR) continue; - - // Process is dead, this is kind of bad.. + + // process we read from has died if (Res == 0) return false; - + // No data - if (Res < 0 && errno == EAGAIN) + if (Res < 0 && (errno == EAGAIN || errno == EWOULDBLOCK)) return true; if (Res < 0) return false; - - End += Res; - - // Look for the end of the message - for (char *I = Buffer; I + 1 < End; I++) + + // extract the message(s) from the buffer + char const *Start = Buffer; + char const * const End = Buffer + Res; + + char const * NL = (char const *) memchr(Start, '\n', End - Start); + if (NL == NULL) { - if (I[1] != '\n' || - (I[0] != '\n' && strncmp(I, "\r\n\r\n", 4) != 0)) - continue; - - // Pull the message out - string Message(Buffer,I-Buffer); - PartialMessage += Message; - - // Fix up the buffer - for (; I < End && (*I == '\n' || *I == '\r'); ++I); - End -= I-Buffer; - memmove(Buffer,I,End-Buffer); - I = Buffer; - - List.push_back(PartialMessage); - PartialMessage.clear(); + // end of buffer: store what we have so far and read new data in + PartialMessage.append(Start, End - Start); + Start = End; } - if (End != Buffer) - { - // If there's text left in the buffer, store it - // in PartialMessage and throw the rest of the buffer - // away. This allows us to handle messages that - // are longer than the static buffer size. - PartialMessage += string(Buffer, End); - End = Buffer; - } else - { - // BUG ALERT: if a message block happens to end at a - // multiple of 64000 characters, this will cause it to - // terminate early, leading to a badly formed block and - // probably crashing the method. However, this is the only - // way we have to find the end of the message block. I have - // an idea of how to fix this, but it will require changes - // to the protocol (essentially to mark the beginning and - // end of the block). - // - // -- dburrows 2008-04-02 - return true; - } + ++NL; + + if (PartialMessage.empty() == false && Start < End) + { + // if we start with a new line, see if the partial message we have ended with one + // so that we properly detect records ending between two read() runs + // cases are: \n|\n , \r\n|\r\n and \r\n\r|\n + // the case \r|\n\r\n is handled by the usual double-newline handling + if ((NL - Start) == 1 || ((NL - Start) == 2 && *Start == '\r')) + { + if (APT::String::Endswith(PartialMessage, "\n") || APT::String::Endswith(PartialMessage, "\r\n\r")) + { + PartialMessage.erase(PartialMessage.find_last_not_of("\r\n") + 1); + List.push_back(PartialMessage); + PartialMessage.clear(); + while (NL < End && (*NL == '\n' || *NL == '\r')) ++NL; + Start = NL; + } + } + } + + while (Start < End) { + char const * NL2 = (char const *) memchr(NL, '\n', End - NL); + if (NL2 == NULL) + { + // end of buffer: store what we have so far and read new data in + PartialMessage.append(Start, End - Start); + break; + } + ++NL2; + + // did we find a double newline? + if ((NL2 - NL) == 1 || ((NL2 - NL) == 2 && *NL == '\r')) + { + PartialMessage.append(Start, NL2 - Start); + PartialMessage.erase(PartialMessage.find_last_not_of("\r\n") + 1); + List.push_back(PartialMessage); + PartialMessage.clear(); + while (NL2 < End && (*NL2 == '\n' || *NL2 == '\r')) ++NL2; + Start = NL2; + } + NL = NL2; + } + + // we have read at least one complete message and nothing left + if (PartialMessage.empty() == true) + return true; if (WaitFd(Fd) == false) return false; - } + } while (true); } /*}}}*/ // MonthConv - Converts a month string into a number /*{{{*/ -- cgit v1.2.3 From 359e1c4f1f8880b62b430f46680df14f94664906 Mon Sep 17 00:00:00 2001 From: David Kalnischkies Date: Sun, 26 Oct 2014 18:47:01 +0100 Subject: move permission changing from -item to -worker The worker is the part closest to the methods, which will call the item methods according to what it gets back from the methods, it is therefore a better place to change permissions as it is very central and can do it now at the point the item is assigned to a method rather than then it is queued for download (and as before while dequeued via Done/Failure). Git-Dch: Ignore --- apt-pkg/acquire-item.cc | 42 ------------------------------------------ apt-pkg/acquire-worker.cc | 37 ++++++++++++++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 45 deletions(-) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index 11c522ae5..ba1669de0 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -44,9 +44,6 @@ #include #include #include -#include -#include -#include #include /*}}}*/ @@ -65,20 +62,6 @@ static void printHashSumComparision(std::string const &URI, HashStringList const std::cerr << "\t- " << hs->toStr() << std::endl; } /*}}}*/ -static void ChangeOwnerAndPermissionOfFile(char const * const requester, char const * const file, char const * const user, char const * const group, mode_t const mode) /*{{{*/ -{ - if (getuid() == 0 && strlen(user) != 0 && strlen(group) != 0) // if we aren't root, we can't chown, so don't try it - { - // ensure the file is owned by root and has good permissions - struct passwd const * const pw = getpwnam(user); - struct group const * const gr = getgrnam(group); - if (pw != NULL && gr != NULL && chown(file, pw->pw_uid, gr->gr_gid) != 0) - _error->WarningE(requester, "chown to %s:%s of file %s failed", user, group, file); - } - if (chmod(file, mode) != 0) - _error->WarningE(requester, "chmod 0%o of file %s failed", mode, file); -} - /*}}}*/ static std::string GetPartialFileName(std::string const &file) /*{{{*/ { std::string DestFile = _config->FindDir("Dir::State::lists") + "partial/"; @@ -155,9 +138,6 @@ pkgAcquire::Item::~Item() fetch this object */ void pkgAcquire::Item::Failed(string Message,pkgAcquire::MethodConfig *Cnf) { - if (RealFileExists(DestFile)) - ChangeOwnerAndPermissionOfFile("Item::Failed", DestFile.c_str(), "root", "root", 0644); - if(ErrorText.empty()) ErrorText = LookupTag(Message,"Message"); UsedMirror = LookupTag(Message,"UsedMirror"); @@ -219,8 +199,6 @@ void pkgAcquire::Item::Done(string Message,unsigned long long Size,HashStringLis if (Owner->Log != 0) Owner->Log->Fetched(Size,atoi(LookupTag(Message,"Resume-Point","0").c_str())); } - if (RealFileExists(DestFile)) - ChangeOwnerAndPermissionOfFile("Item::Done", DestFile.c_str(), "root", "root", 0644); if (FileSize == 0) FileSize= Size; @@ -237,7 +215,6 @@ bool pkgAcquire::Item::Rename(string From,string To) { if (rename(From.c_str(),To.c_str()) == 0) return true; - ChangeOwnerAndPermissionOfFile("Item::Failed", To.c_str(), "root", "root", 0644); std::string S; strprintf(S, _("rename failed, %s (%s -> %s)."), strerror(errno), @@ -249,12 +226,6 @@ bool pkgAcquire::Item::Rename(string From,string To) /*}}}*/ void pkgAcquire::Item::QueueURI(ItemDesc &Item) /*{{{*/ { - if (RealFileExists(DestFile)) - { - std::string SandboxUser = _config->Find("APT::Sandbox::User"); - ChangeOwnerAndPermissionOfFile("Item::QueueURI", DestFile.c_str(), - SandboxUser.c_str(), "root", 0600); - } Owner->Enqueue(Item); } /*}}}*/ @@ -1577,11 +1548,6 @@ void pkgAcqMetaBase::AbortTransaction() // the transaction will abort, so stop anything that is idle if ((*I)->Status == pkgAcquire::Item::StatIdle) (*I)->Status = pkgAcquire::Item::StatDone; - - // reverify might act on a file outside of partial - // (as it itself is good, but needed to verify others, like Release) - if ((*I)->DestFile == (*I)->PartialFile && RealFileExists((*I)->DestFile)) - ChangeOwnerAndPermissionOfFile("AbortTransaction", (*I)->DestFile.c_str(), "root", "root", 0644); } Transaction.clear(); } @@ -2501,11 +2467,7 @@ bool pkgAcqArchive::QueueNext() if ((unsigned long long)Buf.st_size > Version->Size) unlink(DestFile.c_str()); else - { PartialSize = Buf.st_size; - std::string SandboxUser = _config->Find("APT::Sandbox::User"); - ChangeOwnerAndPermissionOfFile("pkgAcqArchive::QueueNext",DestFile.c_str(), SandboxUser.c_str(), "root", 0600); - } } // Disables download of archives - useful if no real installation follows, @@ -2669,11 +2631,7 @@ pkgAcqFile::pkgAcqFile(pkgAcquire *Owner,string URI, HashStringList const &Hashe if ((Size > 0) && (unsigned long long)Buf.st_size > Size) unlink(DestFile.c_str()); else - { PartialSize = Buf.st_size; - std::string SandboxUser = _config->Find("APT::Sandbox::User"); - ChangeOwnerAndPermissionOfFile("pkgAcqFile", DestFile.c_str(), SandboxUser.c_str(), "root", 0600); - } } QueueURI(Desc); diff --git a/apt-pkg/acquire-worker.cc b/apt-pkg/acquire-worker.cc index 64df3c80f..724bdfd49 100644 --- a/apt-pkg/acquire-worker.cc +++ b/apt-pkg/acquire-worker.cc @@ -34,12 +34,29 @@ #include #include #include +#include +#include +#include #include /*}}}*/ using namespace std; +static void ChangeOwnerAndPermissionOfFile(char const * const requester, char const * const file, char const * const user, char const * const group, mode_t const mode) /*{{{*/ +{ + if (getuid() == 0 && strlen(user) != 0 && strlen(group) != 0) // if we aren't root, we can't chown, so don't try it + { + // ensure the file is owned by root and has good permissions + struct passwd const * const pw = getpwnam(user); + struct group const * const gr = getgrnam(group); + if (pw != NULL && gr != NULL && chown(file, pw->pw_uid, gr->gr_gid) != 0) + _error->WarningE(requester, "chown to %s:%s of file %s failed", user, group, file); + } + if (chmod(file, mode) != 0) + _error->WarningE(requester, "chmod 0%o of file %s failed", mode, file); +} + /*}}}*/ // Worker::Worker - Constructor for Queue startup /*{{{*/ // --------------------------------------------------------------------- /* */ @@ -306,7 +323,10 @@ bool pkgAcquire::Worker::RunMessages() pkgAcquire::Item *Owner = Itm->Owner; pkgAcquire::ItemDesc Desc = *Itm; - + + if (RealFileExists(Owner->DestFile)) + ChangeOwnerAndPermissionOfFile("201::URIDone", Owner->DestFile.c_str(), "root", "root", 0644); + // Display update before completion if (Log != 0 && Log->MorePulses == true) Log->Pulse(Owner->GetOwner()); @@ -379,9 +399,13 @@ bool pkgAcquire::Worker::RunMessages() // Display update before completion if (Log != 0 && Log->MorePulses == true) Log->Pulse(Itm->Owner->GetOwner()); - + pkgAcquire::Item *Owner = Itm->Owner; pkgAcquire::ItemDesc Desc = *Itm; + + if (RealFileExists(Owner->DestFile)) + ChangeOwnerAndPermissionOfFile("400::URIFailure", Owner->DestFile.c_str(), "root", "root", 0644); + OwnerQ->ItemDone(Itm); // set some status @@ -542,7 +566,14 @@ bool pkgAcquire::Worker::QueueItem(pkgAcquire::Queue::QItem *Item) } Message += Item->Owner->Custom600Headers(); Message += "\n\n"; - + + if (RealFileExists(Item->Owner->DestFile)) + { + std::string SandboxUser = _config->Find("APT::Sandbox::User"); + ChangeOwnerAndPermissionOfFile("Item::QueueURI", Item->Owner->DestFile.c_str(), + SandboxUser.c_str(), "root", 0600); + } + if (Debug == true) clog << " -> " << Access << ':' << QuoteString(Message,"\n") << endl; OutQueue += Message; -- cgit v1.2.3 From 53702a590c9905cf3104180fefcf126b26e3aad9 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Tue, 4 Nov 2014 14:47:59 +0100 Subject: Call "Dequeue()" for items in AbortTransaction() to fix race The pkgAcquire::Run() code works uses a while(ToFetch > 0) loop over the items queued for fetching. This means that we need to Deqeueue the item if we call AbortTransaction() to avoid a hang. --- apt-pkg/acquire-item.cc | 3 +++ apt-pkg/acquire-item.h | 2 ++ 2 files changed, 5 insertions(+) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-item.cc b/apt-pkg/acquire-item.cc index ba1669de0..f684e81f1 100644 --- a/apt-pkg/acquire-item.cc +++ b/apt-pkg/acquire-item.cc @@ -1547,7 +1547,10 @@ void pkgAcqMetaBase::AbortTransaction() 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(); + } } Transaction.clear(); } diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index 4b4ef5fca..d6eca3e50 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -63,6 +63,8 @@ class pkgAcqMetaBase; */ class pkgAcquire::Item : public WeakPointable { + friend pkgAcqMetaBase; + void *d; protected: -- cgit v1.2.3 From 90d7716a474b4d172b06acfa2320fdc3d8663400 Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Tue, 4 Nov 2014 17:20:52 +0100 Subject: apt-pkg/acquire-item.h: make friend declaration compatible with older gcc --- apt-pkg/acquire-item.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'apt-pkg') diff --git a/apt-pkg/acquire-item.h b/apt-pkg/acquire-item.h index d6eca3e50..ff1bf563a 100644 --- a/apt-pkg/acquire-item.h +++ b/apt-pkg/acquire-item.h @@ -63,7 +63,7 @@ class pkgAcqMetaBase; */ class pkgAcquire::Item : public WeakPointable { - friend pkgAcqMetaBase; + friend class pkgAcqMetaBase; void *d; -- cgit v1.2.3