From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 11 Feb 2020 16:47:41 +0300 (MSK) From: Ivan Zakharyaschev To: ALT Linux Team development discussions In-Reply-To: <6acb6b5b-d135-7071-f500-3142170aefda@altlinux.org> Message-ID: References: <20200129012150.83E7E8440710@gitery.altlinux.org> <45228f59-3529-a3ee-7eb7-67eac012ffda@altlinux.org> <6acb6b5b-d135-7071-f500-3142170aefda@altlinux.org> User-Agent: Alpine 2.20 (LFD 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="1807885841-1250430411-1581428861=:6363" Cc: Anton Farygin , "Vladimir D. Seleznev" Subject: Re: [devel] [SCM] packages/apt: heads/rework-dynamic-mmap X-BeenThere: devel@lists.altlinux.org X-Mailman-Version: 2.1.12 Precedence: list Reply-To: ALT Linux Team development discussions List-Id: ALT Linux Team development discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 11 Feb 2020 13:47:42 -0000 Archived-At: List-Archive: List-Post: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --1807885841-1250430411-1581428861=:6363 Content-Type: text/plain; charset=koi8-r Content-Transfer-Encoding: 8BIT Hello! On Thu, 30 Jan 2020, Aleksei Nikiforov wrote: > > Чтобы не было удивления от бессмысленного значения, я переписал так, как > > ты увидел. Не люблю операции, которые какой-то мусор присваивают, просто > > чтобы место чем-то заполнить. > > > > В моём варианте Prv даже не конструируется и не начинает заполняться, если > > не удалось выделить память в Map под строку Version. Это же понятнее и > > лишних вопросов не вызвает: что потом с этой частично осмысленно > > заполненной, частично незаполненной структурой произойдёт и кто её в таком > > виде увидит дальше. > > > > По сути установление нуля - это лишь сохранение старого поведения. Если вызов > WriteString(Version) возвращал ноль, то сначала этот ноль присваивался > Prv->ProvideVersion, а уже затем проверялось значение Prv->ProvideVersion. С > новым API в случае ошибки возвращается особое значение, пустой > std::experimental::optional, но для сохранения старого поведения данной > функции pkgCacheGenerator::ListParser::NewProvides ноль присваивается > переменной Prv->ProvideVersion. Я не счёл необходимым сильнее менять данную > функцию, по крайней мере до сих пор, но если есть желание дальше изменять > данный код - я не возражаю. Теперь в своей ветке optional учёл это высказанное соображение и прочие мелкие замечания, которые тут высказывались к моим редакциям этих изменений в виде коммитов. В этом месте сначала делаю ближе к старому поведению, потом в другом коммите меняю сильнее. Теперь вся серия выглядит так: $ git --no-pager log -p base..optional commit 9146fbb5ccf224505334190472fe7ddfc1531e4e Author: Ivan Zakharyaschev Date: Mon Feb 10 13:03:07 2020 +0300 .dir-locals.el: apply our Emacs mode settings to .h files, too diff --git a/apt/.dir-locals.el b/apt/.dir-locals.el index c1497844e..acc107299 100644 --- a/apt/.dir-locals.el +++ b/apt/.dir-locals.el @@ -2,4 +2,9 @@ ;;; For more information see (info "(emacs) Directory Variables") ((c++-mode + (c-basic-offset . 3)) + + ;; For switching in .h headers: + (c-mode + (mode . c++) (c-basic-offset . 3))) commit f9de67b18e05656ec43df41cde61dbb338fd01f4 Author: Ivan Zakharyaschev Date: Mon Feb 10 17:02:20 2020 +0300 DynamicMMap::RawAllocate(): do not modify data on failure diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc index ef9e31242..d387560e8 100644 --- a/apt/apt-pkg/contrib/mmap.cc +++ b/apt/apt-pkg/contrib/mmap.cc @@ -207,9 +207,7 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln) unsigned long long Result = iSize; if (Aln != 0) Result += Aln - (iSize%Aln); - - iSize = Result + Size; - + // Just in case error check if (Result + Size > WorkSpace) { @@ -217,6 +215,8 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln) return 0; } + iSize = Result + Size; + return Result; } /*}}}*/ commit 5fa3ac151d7b5290a775e69f7e79dce0837db2d8 Author: Ivan Zakharyaschev Date: Mon Feb 10 17:03:42 2020 +0300 DynamicMMap::RawAllocate(): no offset needed if already aligned (was off-by-one) diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc index d387560e8..44cc6bae4 100644 --- a/apt/apt-pkg/contrib/mmap.cc +++ b/apt/apt-pkg/contrib/mmap.cc @@ -206,7 +206,7 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln) { unsigned long long Result = iSize; if (Aln != 0) - Result += Aln - (iSize%Aln); + Result += Aln - (iSize%Aln ? : Aln); // Just in case error check if (Result + Size > WorkSpace) commit ca6142850ccea2f726bb59295f16fe5a0820a562 Author: Ivan Zakharyaschev Date: Mon Feb 10 19:29:34 2020 +0300 DynamicMMap::WriteString(): fix the check for "out of room" when length is automatic It made no sense to make the check before we have the valid value in Len. diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc index 44cc6bae4..6b56775a1 100644 --- a/apt/apt-pkg/contrib/mmap.cc +++ b/apt/apt-pkg/contrib/mmap.cc @@ -271,6 +271,9 @@ unsigned long DynamicMMap::Allocate(unsigned long ItemSize) unsigned long DynamicMMap::WriteString(const char *String, unsigned long Len) { + if (Len == (unsigned long)-1) + Len = strlen(String); + unsigned long Result = iSize; // Just in case error check if (Result + Len > WorkSpace) @@ -279,8 +282,6 @@ unsigned long DynamicMMap::WriteString(const char *String, return 0; } - if (Len == (unsigned long)-1) - Len = strlen(String); iSize += Len + 1; memcpy((char *)Base + Result,String,Len); ((char *)Base)[Result + Len] = 0; commit e785f0e8636e47a672445e70f2923a5eea566b33 Author: Ivan Zakharyaschev Date: Wed Jan 29 04:41:13 2020 +0300 use the safer C++-style static_cast instead of a C-style cast (from void*) What is happening here: Map->RawAllocate() returns the index in an array of bytes (i.e., of char; no matter whether they are (un)signed); therefore, we cast the base pointer to the corresponding type, so that the pointer arithmetic gives a pointer to the beginning of the allocated space. We do not want to rely on non-standard void*-arithmetic. diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc index 070c0f040..16d3592e9 100644 --- a/apt/apt-pkg/pkgcachegen.cc +++ b/apt/apt-pkg/pkgcachegen.cc @@ -882,7 +882,7 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, { // Preload the map with the source cache FileFd SCacheF(SrcCacheFile,FileFd::ReadOnly); - if (SCacheF.Read((unsigned char *)Map->Data() + Map->RawAllocate(SCacheF.Size()), + if (SCacheF.Read(static_cast(Map->Data()) + Map->RawAllocate(SCacheF.Size()), SCacheF.Size()) == false) return false; commit 4d297001f6d3ddc3e12cf4b950e821d867b77092 Author: Ivan Zakharyaschev Date: Wed Jan 29 05:02:13 2020 +0300 rigorously compute HeaderP as the pointer to the beginning of Map The old code expected that the first call to Map.RawAllocate() would give the pointer to the very beginning of Map, i.e., the same as Map.Data(). We rewrite this code without such presupposition: no matter what the logic behind Map.RawAllocate() will ever be, we'll actually get the pointer to the space which is being "allocated" by this call inside Map. We use the safer C++-style static_cast (from void*) where possible. What is happening here with the pointer arithmetic: Map->RawAllocate() returns the index in an array of bytes (i.e., of char); therefore, we cast the base pointer to the corresponding type, so that the pointer arithmetic gives the correct pointer to the beginning of the "allocated" space. We do not want to rely on non-standard void*-arithmetic. diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc index 16d3592e9..8bf5ed639 100644 --- a/apt/apt-pkg/pkgcachegen.cc +++ b/apt/apt-pkg/pkgcachegen.cc @@ -54,9 +54,10 @@ pkgCacheGenerator::pkgCacheGenerator(DynamicMMap *pMap,OpProgress *Prog) : if (Map.Size() == 0) { + const auto idxBeginning = Map.RawAllocate(sizeof(pkgCache::Header)); + // Setup the map interface.. - Cache.HeaderP = (pkgCache::Header *)Map.Data(); - Map.RawAllocate(sizeof(pkgCache::Header)); + Cache.HeaderP = reinterpret_cast(static_cast(Map.Data()) + idxBeginning); Map.UsePools(*Cache.HeaderP->Pools,sizeof(Cache.HeaderP->Pools)/sizeof(Cache.HeaderP->Pools[0])); // Starting header commit 063c9051ddc8d736193733f19ab9a821c30ce3b8 Author: Ivan Zakharyaschev Date: Wed Jan 29 20:41:33 2020 +0300 Use special type to return allocation failure since 0 is a valid offset value This change is heavily based on: commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6 Author: Aleksei Nikiforov Date: Thu Jun 13 12:35:49 2019 +0300 Use special type to return allocation failure since 0 is a valid offset value which has been rebased and purified a bit (so that the code clearly matches the intention at the changed places). Co-Authored-by: Vladimir D. Seleznev . diff --git a/apt.spec b/apt.spec index 362179ec1..4b18a9453 100644 --- a/apt.spec +++ b/apt.spec @@ -230,7 +230,7 @@ printf '%_target_cpu\t%_target_cpu' >> buildlib/archtable %autoreconf %add_optflags -DAPTRPM_ID=\\\"%name-%{?epoch:%epoch:}%version-%release%{?disttag::%disttag}.%_target_cpu\\\" %ifarch %e2k -%add_optflags -std=gnu++11 +%add_optflags -std=c++14 %endif %configure --includedir=%_includedir/apt-pkg %{subst_enable static} diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc index 6b56775a1..77aad3656 100644 --- a/apt/apt-pkg/contrib/mmap.cc +++ b/apt/apt-pkg/contrib/mmap.cc @@ -202,7 +202,7 @@ DynamicMMap::~DynamicMMap() // DynamicMMap::RawAllocate - Allocate a raw chunk of unaligned space /*{{{*/ // --------------------------------------------------------------------- /* This allocates a block of memory aligned to the given size */ -unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln) +std::experimental::optional DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln) { unsigned long long Result = iSize; if (Aln != 0) @@ -212,7 +212,7 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln) if (Result + Size > WorkSpace) { _error->Error("Dynamic MMap ran out of room"); - return 0; + return std::experimental::nullopt; } iSize = Result + Size; @@ -224,8 +224,8 @@ unsigned long DynamicMMap::RawAllocate(unsigned long Size,unsigned long Aln) // --------------------------------------------------------------------- /* This allocates an Item of size ItemSize so that it is aligned to its size in the file. */ -unsigned long DynamicMMap::Allocate(unsigned long ItemSize) -{ +std::experimental::optional DynamicMMap::Allocate(unsigned long ItemSize) +{ // Look for a matching pool entry Pool *I; Pool *Empty = 0; @@ -244,7 +244,7 @@ unsigned long DynamicMMap::Allocate(unsigned long ItemSize) if (Empty == 0) { _error->Error("Ran out of allocation pools"); - return 0; + return std::experimental::nullopt; } I = Empty; @@ -256,8 +256,14 @@ unsigned long DynamicMMap::Allocate(unsigned long ItemSize) if (I->Count == 0) { I->Count = 20*1024/ItemSize; - I->Start = RawAllocate(I->Count*ItemSize,ItemSize); - } + auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize); + + // Has the allocation failed? + if (!idxResult) + return idxResult; + + I->Start = *idxResult; + } I->Count--; unsigned long Result = I->Start; @@ -268,7 +274,7 @@ unsigned long DynamicMMap::Allocate(unsigned long ItemSize) // DynamicMMap::WriteString - Write a string to the file /*{{{*/ // --------------------------------------------------------------------- /* Strings are not aligned to anything */ -unsigned long DynamicMMap::WriteString(const char *String, +std::experimental::optional DynamicMMap::WriteString(const char *String, unsigned long Len) { if (Len == (unsigned long)-1) @@ -279,7 +285,7 @@ unsigned long DynamicMMap::WriteString(const char *String, if (Result + Len > WorkSpace) { _error->Error("Dynamic MMap ran out of room"); - return 0; + return std::experimental::nullopt; } iSize += Len + 1; diff --git a/apt/apt-pkg/contrib/mmap.h b/apt/apt-pkg/contrib/mmap.h index efc5245cb..14df0550c 100644 --- a/apt/apt-pkg/contrib/mmap.h +++ b/apt/apt-pkg/contrib/mmap.h @@ -29,6 +29,7 @@ #endif #include +#include #include using std::string; @@ -90,10 +91,10 @@ class DynamicMMap : public MMap public: // Allocation - unsigned long RawAllocate(unsigned long Size,unsigned long Aln = 0); - unsigned long Allocate(unsigned long ItemSize); - unsigned long WriteString(const char *String,unsigned long Len = (unsigned long)-1); - inline unsigned long WriteString(const string &S) {return WriteString(S.c_str(),S.length());}; + std::experimental::optional RawAllocate(unsigned long Size,unsigned long Aln = 0); + std::experimental::optional Allocate(unsigned long ItemSize); + std::experimental::optional WriteString(const char *String,unsigned long Len = (unsigned long)-1); + inline std::experimental::optional WriteString(const string &S) {return WriteString(S.c_str(),S.length());}; void UsePools(Pool &P,unsigned int Count) {Pools = &P; PoolCount = Count;}; DynamicMMap(FileFd &F,unsigned long Flags,unsigned long WorkSpace = 2*1024*1024); diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc index 8bf5ed639..4c6055cbb 100644 --- a/apt/apt-pkg/pkgcachegen.cc +++ b/apt/apt-pkg/pkgcachegen.cc @@ -55,16 +55,20 @@ pkgCacheGenerator::pkgCacheGenerator(DynamicMMap *pMap,OpProgress *Prog) : if (Map.Size() == 0) { const auto idxBeginning = Map.RawAllocate(sizeof(pkgCache::Header)); + const auto idxVerSysName = WriteStringInMap(_system->VS->Label); + const auto idxArch = WriteStringInMap(_config->Find("APT::Architecture")); + if ((!idxBeginning) || (!idxVerSysName) || (!idxArch)) + return; // Setup the map interface.. - Cache.HeaderP = reinterpret_cast(static_cast(Map.Data()) + idxBeginning); + Cache.HeaderP = reinterpret_cast(static_cast(Map.Data()) + *idxBeginning); Map.UsePools(*Cache.HeaderP->Pools,sizeof(Cache.HeaderP->Pools)/sizeof(Cache.HeaderP->Pools[0])); // Starting header *Cache.HeaderP = pkgCache::Header(); - Cache.HeaderP->VerSysName = WriteStringInMap(_system->VS->Label); - Cache.HeaderP->Architecture = WriteStringInMap(_config->Find("APT::Architecture")); - Cache.ReMap(); + Cache.HeaderP->VerSysName = *idxVerSysName; + Cache.HeaderP->Architecture = *idxArch; + Cache.ReMap(); } else { @@ -98,17 +102,17 @@ pkgCacheGenerator::~pkgCacheGenerator() } /*}}}*/ // CacheGenerator::WriteStringInMap /*{{{*/ -unsigned long pkgCacheGenerator::WriteStringInMap(const char *String, +std::experimental::optional pkgCacheGenerator::WriteStringInMap(const char *String, unsigned long Len) { return Map.WriteString(String, Len); } /*}}}*/ // CacheGenerator::WriteStringInMap /*{{{*/ -unsigned long pkgCacheGenerator::WriteStringInMap(const char *String) { +std::experimental::optional pkgCacheGenerator::WriteStringInMap(const char *String) { return Map.WriteString(String); } /*}}}*/ -unsigned long pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/ +std::experimental::optional pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/ return Map.Allocate(size); } /*}}}*/ @@ -222,7 +226,12 @@ bool pkgCacheGenerator::MergeList(ListParser &List, } // Add a new version - *Last = NewVersion(Ver,Version,*Last); + const auto verindex = NewVersion(Ver,Version,*Last); + if (!verindex) + return _error->Error(_("Error occurred while processing %s (NewVersion0)"), + PackageName.c_str()); + *Last = *verindex; + Ver->ParentPkg = Pkg.Index(); Ver->Hash = Hash; if (List.NewVersion(Ver) == false) @@ -366,21 +375,20 @@ bool pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, const string &Nam #endif // Get a structure - unsigned long Package = AllocateInMap(sizeof(pkgCache::Package)); - if (Package == 0) + const auto Package = AllocateInMap(sizeof(pkgCache::Package)); + const auto idxName = WriteStringInMap(Name); + if ((!Package) || (!idxName)) return false; - - Pkg = pkgCache::PkgIterator(Cache,Cache.PkgP + Package); - + + Pkg = pkgCache::PkgIterator(Cache,Cache.PkgP + *Package); + // Insert it into the hash table unsigned long Hash = Cache.Hash(Name); Pkg->NextPackage = Cache.HeaderP->HashTable[Hash]; - Cache.HeaderP->HashTable[Hash] = Package; - + Cache.HeaderP->HashTable[Hash] = *Package; + // Set the name and the ID - Pkg->Name = WriteStringInMap(Name); - if (Pkg->Name == 0) - return false; + Pkg->Name = *idxName; Pkg->ID = Cache.HeaderP->PackageCount++; return true; @@ -408,7 +416,11 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, if ( (':' - '@' < 0 ? rc < 0 : rc > 0) || (rc != 0 && List.IsDatabase()) ) { - Ver->VerStr = WriteStringInMap(Version); + const auto verStrIdx = WriteStringInMap(Version); + if (!verStrIdx) + return false; + + Ver->VerStr = *verStrIdx; } } @@ -416,11 +428,11 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, return true; // Get a structure - unsigned long VerFile = AllocateInMap(sizeof(pkgCache::VerFile)); - if (VerFile == 0) - return 0; + const auto VerFile = AllocateInMap(sizeof(pkgCache::VerFile)); + if (!VerFile) + return false; - pkgCache::VerFileIterator VF(Cache,Cache.VerFileP + VerFile); + pkgCache::VerFileIterator VF(Cache,Cache.VerFileP + *VerFile); VF->File = CurrentFile - Cache.PkgFileP; // Link it to the end of the list @@ -442,23 +454,22 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, // CacheGenerator::NewVersion - Create a new Version /*{{{*/ // --------------------------------------------------------------------- /* This puts a version structure in the linked list */ -unsigned long pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, +std::experimental::optional pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, const string &VerStr, unsigned long Next) { // Get a structure - unsigned long Version = AllocateInMap(sizeof(pkgCache::Version)); - if (Version == 0) - return 0; - + const auto Version = AllocateInMap(sizeof(pkgCache::Version)); + const auto idxVerStr = WriteStringInMap(VerStr); + if ((!Version) || (!idxVerStr)) + return std::experimental::nullopt; + // Fill it in - Ver = pkgCache::VerIterator(Cache,Cache.VerP + Version); + Ver = pkgCache::VerIterator(Cache,Cache.VerP + *Version); Ver->NextVer = Next; Ver->ID = Cache.HeaderP->VersionCount++; - Ver->VerStr = WriteStringInMap(VerStr); - if (Ver->VerStr == 0) - return 0; - + Ver->VerStr = *idxVerStr; + return Version; } /*}}}*/ @@ -475,12 +486,12 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver, pkgCache &Cache = Owner->Cache; // Get a structure - unsigned long Dependency = Owner->AllocateInMap(sizeof(pkgCache::Dependency)); - if (Dependency == 0) + const auto Dependency = Owner->AllocateInMap(sizeof(pkgCache::Dependency)); + if (!Dependency) return false; // Fill it in - pkgCache::DepIterator Dep(Cache,Cache.DepP + Dependency); + pkgCache::DepIterator Dep(Cache,Cache.DepP + *Dependency); Dep->ParentVer = Ver.Index(); Dep->Type = Type; Dep->CompareOp = Op; @@ -498,8 +509,13 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver, if (I->Version != 0 && I.TargetVer() == Version) Dep->Version = I->Version;*/ if (Dep->Version == 0) - if ((Dep->Version = WriteString(Version)) == 0) - return false; + { + const auto index = WriteString(Version); + if (!index) + return false; + + Dep->Version = *index; + } } // Link it to the package @@ -545,18 +561,29 @@ bool pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver, #endif // Get a structure - unsigned long Provides = Owner->AllocateInMap(sizeof(pkgCache::Provides)); - if (Provides == 0) + const auto Provides = Owner->AllocateInMap(sizeof(pkgCache::Provides)); + if (!Provides) return false; Cache.HeaderP->ProvidesCount++; // Fill it in - pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + Provides,Cache.PkgP); + pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + *Provides,Cache.PkgP); Prv->Version = Ver.Index(); Prv->NextPkgProv = Ver->ProvidesList; Ver->ProvidesList = Prv.Index(); - if (Version.empty() == false && (Prv->ProvideVersion = WriteString(Version)) == 0) - return false; + + if (Version.empty() == false) + { + const auto idxVersion = WriteString(Version); + // FIXME: If this allocation fails, did it make sense to fill in Prv above? + if (!idxVersion) + { + // FIXME: Does it really make sense to fill it in on failure? + Prv->ProvideVersion = 0; + return false; + } + Prv->ProvideVersion = *idxVersion; + } // Locate the target package pkgCache::PkgIterator Pkg; @@ -580,23 +607,24 @@ bool pkgCacheGenerator::SelectFile(const string &File, const string &Site, unsigned long Flags) { // Get some space for the structure - CurrentFile = Cache.PkgFileP + AllocateInMap(sizeof(*CurrentFile)); - if (CurrentFile == Cache.PkgFileP) + const auto idxFile = AllocateInMap(sizeof(*CurrentFile)); + const auto idxFileName = WriteStringInMap(File); + const auto idxSite = WriteUniqString(Site); + const auto idxIndexType = WriteUniqString(Index.GetType()->Label); + if ((!idxFile) || (!idxFileName) || (!idxSite) || (!idxIndexType)) return false; + CurrentFile = Cache.PkgFileP + *idxFile; // Fill it in - CurrentFile->FileName = WriteStringInMap(File); - CurrentFile->Site = WriteUniqString(Site); + CurrentFile->FileName = *idxFileName; + CurrentFile->Site = *idxSite; CurrentFile->NextFile = Cache.HeaderP->FileList; CurrentFile->Flags = Flags; CurrentFile->ID = Cache.HeaderP->PackageFileCount; - CurrentFile->IndexType = WriteUniqString(Index.GetType()->Label); + CurrentFile->IndexType = *idxIndexType; PkgFileName = File; Cache.HeaderP->FileList = CurrentFile - Cache.PkgFileP; Cache.HeaderP->PackageFileCount++; - - if (CurrentFile->FileName == 0) - return false; if (Progress != 0) Progress->SubProgress(Index.Size()); @@ -607,7 +635,7 @@ bool pkgCacheGenerator::SelectFile(const string &File, const string &Site, // --------------------------------------------------------------------- /* This is used to create handles to strings. Given the same text it always returns the same number */ -unsigned long pkgCacheGenerator::WriteUniqString(const char *S, +std::experimental::optional pkgCacheGenerator::WriteUniqString(const char *S, unsigned int Size) { /* We use a very small transient hash table here, this speeds up generation @@ -637,18 +665,17 @@ unsigned long pkgCacheGenerator::WriteUniqString(const char *S, } // Get a structure - unsigned long Item = AllocateInMap(sizeof(pkgCache::StringItem)); - if (Item == 0) - return 0; + const auto Item = AllocateInMap(sizeof(pkgCache::StringItem)); + const auto idxString = WriteStringInMap(S, Size); + if ((!Item) || (!idxString)) + return std::experimental::nullopt; // Fill in the structure - pkgCache::StringItem *ItemP = Cache.StringItemP + Item; + pkgCache::StringItem *ItemP = Cache.StringItemP + *Item; ItemP->NextItem = I - Cache.StringItemP; - *Last = Item; - ItemP->String = WriteStringInMap(S,Size); - if (ItemP->String == 0) - return 0; - + *Last = *Item; + ItemP->String = *idxString; + Bucket = ItemP; return ItemP->String; } @@ -883,7 +910,10 @@ bool pkgMakeStatusCache(pkgSourceList &List,OpProgress &Progress, { // Preload the map with the source cache FileFd SCacheF(SrcCacheFile,FileFd::ReadOnly); - if (SCacheF.Read(static_cast(Map->Data()) + Map->RawAllocate(SCacheF.Size()), + const auto idxAllocate = Map->RawAllocate(SCacheF.Size()); + if (!idxAllocate) + return false; + if (SCacheF.Read(static_cast(Map->Data()) + *idxAllocate, SCacheF.Size()) == false) return false; diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h index a187cd1e8..69c4f2ea1 100644 --- a/apt/apt-pkg/pkgcachegen.h +++ b/apt/apt-pkg/pkgcachegen.h @@ -24,6 +24,8 @@ #include +#include + class pkgSourceList; class OpProgress; class MMap; @@ -34,10 +36,10 @@ class pkgCacheGenerator private: pkgCache::StringItem *UniqHash[26]; - unsigned long WriteStringInMap(const std::string &String) { return WriteStringInMap(String.c_str(), String.length()); }; - unsigned long WriteStringInMap(const char *String); - unsigned long WriteStringInMap(const char *String, unsigned long Len); - unsigned long AllocateInMap(unsigned long size); + std::experimental::optional WriteStringInMap(const std::string &String) { return WriteStringInMap(String.c_str(), String.length()); }; + std::experimental::optional WriteStringInMap(const char *String); + std::experimental::optional WriteStringInMap(const char *String, unsigned long Len); + std::experimental::optional AllocateInMap(unsigned long size); public: @@ -57,15 +59,15 @@ class pkgCacheGenerator bool FoundFileDeps; bool NewFileVer(pkgCache::VerIterator &Ver,ListParser &List); - unsigned long NewVersion(pkgCache::VerIterator &Ver,const string &VerStr,unsigned long Next); + std::experimental::optional NewVersion(pkgCache::VerIterator &Ver,const string &VerStr,unsigned long Next); public: // CNC:2003-02-27 - We need this in rpmListParser. bool NewPackage(pkgCache::PkgIterator &PkgI,const string &Pkg); - unsigned long WriteUniqString(const char *S,unsigned int Size); - inline unsigned long WriteUniqString(const string &S) {return WriteUniqString(S.c_str(),S.length());}; + std::experimental::optional WriteUniqString(const char *S,unsigned int Size); + inline std::experimental::optional WriteUniqString(const string &S) {return WriteUniqString(S.c_str(),S.length());}; void DropProgress() {Progress = 0;}; bool SelectFile(const string &File,const string &Site,pkgIndexFile const &Index, @@ -101,10 +103,10 @@ class pkgCacheGenerator::ListParser pkgCacheGenerator *Owner; friend class pkgCacheGenerator; - inline unsigned long WriteUniqString(const string &S) {return Owner->WriteUniqString(S);}; - inline unsigned long WriteUniqString(const char *S,unsigned int Size) {return Owner->WriteUniqString(S,Size);}; - inline unsigned long WriteString(const string &S) {return Owner->WriteStringInMap(S);}; - inline unsigned long WriteString(const char *S,unsigned int Size) {return Owner->WriteStringInMap(S,Size);}; + inline std::experimental::optional WriteUniqString(const string &S) {return Owner->WriteUniqString(S);}; + inline std::experimental::optional WriteUniqString(const char *S,unsigned int Size) {return Owner->WriteUniqString(S,Size);}; + inline std::experimental::optional WriteString(const string &S) {return Owner->WriteStringInMap(S);}; + inline std::experimental::optional WriteString(const char *S,unsigned int Size) {return Owner->WriteStringInMap(S,Size);}; bool NewDepends(pkgCache::VerIterator &Ver, const string &Package, const string &Version,unsigned int Op, unsigned int Type); diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc index 2fc4e01b8..b3d18d7e6 100644 --- a/apt/apt-pkg/rpm/rpmlistparser.cc +++ b/apt/apt-pkg/rpm/rpmlistparser.cc @@ -70,7 +70,7 @@ rpmListParser::~rpmListParser() // ListParser::UniqFindTagWrite - Find the tag and write a unq string /*{{{*/ // --------------------------------------------------------------------- /* */ -unsigned long rpmListParser::UniqFindTagWrite(int Tag) +std::experimental::optional rpmListParser::UniqFindTagWrite(int Tag) { char *Start; char *Stop; @@ -79,7 +79,7 @@ unsigned long rpmListParser::UniqFindTagWrite(int Tag) void *data; if (headerGetEntry(header, Tag, &type, &data, &count) != 1) - return 0; + return std::experimental::nullopt; if (type == RPM_STRING_TYPE) { @@ -255,8 +255,13 @@ bool rpmListParser::NewVersion(pkgCache::VerIterator &Ver) #endif // Parse the section - Ver->Section = UniqFindTagWrite(RPMTAG_GROUP); - Ver->Arch = UniqFindTagWrite(RPMTAG_ARCH); + const auto idxSection = UniqFindTagWrite(RPMTAG_GROUP); + const auto idxArch = UniqFindTagWrite(RPMTAG_ARCH); + if ((!idxSection) || (!idxArch)) + return false; + + Ver->Section = *idxSection; + Ver->Arch = *idxArch; // Archive Size Ver->Size = Handler->FileSize(); @@ -296,7 +301,14 @@ bool rpmListParser::UsePackage(pkgCache::PkgIterator &Pkg, if (SeenPackages != NULL) (*SeenPackages)[Pkg.Name()] = true; if (Pkg->Section == 0) - Pkg->Section = UniqFindTagWrite(RPMTAG_GROUP); + { + const auto idxSection = UniqFindTagWrite(RPMTAG_GROUP); + if (!idxSection) + return false; + + Pkg->Section = *idxSection; + } + if (_error->PendingError()) return false; string PkgName = Pkg.Name(); @@ -740,19 +752,61 @@ bool rpmListParser::LoadReleaseInfo(pkgCache::PkgFileIterator &FileI, const char *Start; const char *Stop; + if (Section.Find("Archive",Start,Stop)) - FileI->Archive = WriteUniqString(Start,Stop - Start); + { + const auto idx = WriteUniqString(Start, Stop - Start); + if (!idx) + return false; + + FileI->Archive = *idx; + } + if (Section.Find("Component",Start,Stop)) - FileI->Component = WriteUniqString(Start,Stop - Start); + { + const auto idx = WriteUniqString(Start, Stop - Start); + if (!idx) + return false; + + FileI->Component = *idx; + } + if (Section.Find("Version",Start,Stop)) - FileI->Version = WriteUniqString(Start,Stop - Start); + { + const auto idx = WriteUniqString(Start, Stop - Start); + if (!idx) + return false; + + FileI->Version = *idx; + } + if (Section.Find("Origin",Start,Stop)) - FileI->Origin = WriteUniqString(Start,Stop - Start); + { + const auto idx = WriteUniqString(Start, Stop - Start); + if (!idx) + return false; + + FileI->Origin = *idx; + } + if (Section.Find("Label",Start,Stop)) - FileI->Label = WriteUniqString(Start,Stop - Start); + { + const auto idx = WriteUniqString(Start, Stop - Start); + if (!idx) + return false; + + FileI->Label = *idx; + } + if (Section.Find("Architecture",Start,Stop)) - FileI->Architecture = WriteUniqString(Start,Stop - Start); - + { + const auto idx = WriteUniqString(Start, Stop - Start); + if (!idx) + return false; + + FileI->Architecture = *idx; + } + if (Section.FindFlag("NotAutomatic",FileI->Flags, pkgCache::Flag::NotAutomatic) == false) _error->Warning(_("Bad NotAutomatic flag")); diff --git a/apt/apt-pkg/rpm/rpmlistparser.h b/apt/apt-pkg/rpm/rpmlistparser.h index 5c6173c0a..dd57f7c9b 100644 --- a/apt/apt-pkg/rpm/rpmlistparser.h +++ b/apt/apt-pkg/rpm/rpmlistparser.h @@ -20,6 +20,7 @@ #include #include #include +#include using namespace std; @@ -45,7 +46,7 @@ class rpmListParser : public pkgCacheGenerator::ListParser bool Duplicated; - unsigned long UniqFindTagWrite(int Tag); + std::experimental::optional UniqFindTagWrite(int Tag); bool ParseStatus(pkgCache::PkgIterator &Pkg,pkgCache::VerIterator &Ver); bool ParseDepends(pkgCache::VerIterator &Ver, char **namel, char **verl, int32_t *flagl, commit 555697737ef0f0280ba7521fe788ce8b4ebab492 Author: Ivan Zakharyaschev Date: Thu Jan 30 15:40:00 2020 +0300 don't even start to fill in a structure, if we can't allocate the prerequisites FIXME: There are other similar placase in this code; have a look at them and possibly re-organize. (E.g., pkgCacheGenerator::ListParser::NewDepends(), apart from pkgCacheGenerator::ListParser::NewProvides() already changed here.) diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc index 4c6055cbb..fe0a4e86e 100644 --- a/apt/apt-pkg/pkgcachegen.cc +++ b/apt/apt-pkg/pkgcachegen.cc @@ -559,11 +559,27 @@ bool pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver, if (Ver.ParentPkg().Name() == PackageName) return true; #endif - + + // used only if !Version.empty() + std::experimental::optional idxVersion = std::experimental::nullopt; + // Try to allocate the prerequisite string + if (Version.empty() == false) { + idxVersion = WriteString(Version); + if (!idxVersion) + { + /* Don't even start to fill in the structure below, + if we can't get the prerequisite. */ + return false; + } + } // Get a structure const auto Provides = Owner->AllocateInMap(sizeof(pkgCache::Provides)); if (!Provides) return false; + /* Note: + this accounting is done only if all the prerequistes have been allocated + and hence the structure will actually be used (filled in). + */ Cache.HeaderP->ProvidesCount++; // Fill it in @@ -572,16 +588,10 @@ bool pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver, Prv->NextPkgProv = Ver->ProvidesList; Ver->ProvidesList = Prv.Index(); - if (Version.empty() == false) + if (idxVersion) { - const auto idxVersion = WriteString(Version); - // FIXME: If this allocation fails, did it make sense to fill in Prv above? - if (!idxVersion) - { - // FIXME: Does it really make sense to fill it in on failure? - Prv->ProvideVersion = 0; - return false; - } + /* FIXME: If Version.empty(), this field has never been touched. + Is this correct? */ Prv->ProvideVersion = *idxVersion; } commit b06f43d3c951ba7b2da8108516b551f1207beed6 Author: Ivan Zakharyaschev Date: Wed Jan 29 21:40:53 2020 +0300 DynamicMMap::RawAllocate(): error on "Can't allocate an item of size zero" Picked this single change from: commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b Author: Aleksei Nikiforov Date: Tue Jun 11 14:53:47 2019 +0300 apt-pkg/pkgcachegen.{cc,h} changes make the used MMap moveable (and therefore dynamic resizeable) by applying (some) mad pointer magic Ported from Apt from Debian and adapted to the new return type. diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc index 77aad3656..ad49326df 100644 --- a/apt/apt-pkg/contrib/mmap.cc +++ b/apt/apt-pkg/contrib/mmap.cc @@ -226,6 +226,12 @@ std::experimental::optional DynamicMMap::RawAllocate(unsigned lon size in the file. */ std::experimental::optional DynamicMMap::Allocate(unsigned long ItemSize) { + if (ItemSize == 0) + { + _error->Error("Can't allocate an item of size zero"); + return std::experimental::nullopt; + } + // Look for a matching pool entry Pool *I; Pool *Empty = 0; commit b92df3199a5eda44f5108e6721c5cb76d946d0fc Author: Ivan Zakharyaschev Date: Thu Jan 30 01:28:56 2020 +0300 rpmindexfile.cc rpmPkgListIndex::Merge(): drop a bogus call to FileFd::Seek() This operation couldn't have any noticeable effect, because the corresponding FileFd object is about to be destroyed: it's the end of its scope. (Note that it was declared and constructed 4 lines above. This explanation was given by darktemplar@ and quoted in https://lists.altlinux.org/pipermail/devel/2020-January/209736.html .) This single change has been picked from the following changeset (which otherwise has a different common purpose): commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6 Author: Aleksei Nikiforov Date: Thu Jun 13 12:35:49 2019 +0300 Use special type to return allocation failure since 0 is a valid offset value This change will slightly alleviate the rewriting of code at this place for rigorous error-handling (which is a change also present in the cited-above commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6, but not related to its purpose). diff --git a/apt/apt-pkg/rpm/rpmindexfile.cc b/apt/apt-pkg/rpm/rpmindexfile.cc index 271cd8f83..a66d4482c 100644 --- a/apt/apt-pkg/rpm/rpmindexfile.cc +++ b/apt/apt-pkg/rpm/rpmindexfile.cc @@ -407,7 +407,6 @@ bool rpmPkgListIndex::Merge(pkgCacheGenerator &Gen,OpProgress &Prog) const if (_error->PendingError() == true) return false; Parser.LoadReleaseInfo(File,Rel); - Rel.Seek(0); } return true; commit 7db04197e0029146ace692a4fa9ee7088dd76b8a (@ALT/optional, optional) Author: Ivan Zakharyaschev Date: Thu Jan 30 04:44:06 2020 +0300 rpmindexfile.cc rpmPkgListIndex::Merge(): rigorously handle errors from Parser This single change has been picked from the following changeset (which otherwise has a different common purpose): commit 667677a7d268aaf0f84ce3bc0567c5350db20ec6 Author: Aleksei Nikiforov Date: Thu Jun 13 12:35:49 2019 +0300 Use special type to return allocation failure since 0 is a valid offset value Unfortunately, that commit doesn't explain why the new behavior is better than the old one (ignoring the error from Parser). diff --git a/apt/apt-pkg/rpm/rpmindexfile.cc b/apt/apt-pkg/rpm/rpmindexfile.cc index a66d4482c..5ba42155c 100644 --- a/apt/apt-pkg/rpm/rpmindexfile.cc +++ b/apt/apt-pkg/rpm/rpmindexfile.cc @@ -406,7 +406,8 @@ bool rpmPkgListIndex::Merge(pkgCacheGenerator &Gen,OpProgress &Prog) const FileFd Rel(RelFile,FileFd::ReadOnly); if (_error->PendingError() == true) return false; - Parser.LoadReleaseInfo(File,Rel); + if (!Parser.LoadReleaseInfo(File,Rel)) + return false; } return true; -- Best regards, Ivan --1807885841-1250430411-1581428861=:6363--