From c3587c0d9de852eca11d9bbc004095d54115eda4 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 24 Feb 2020 17:08:34 +0100 Subject: Replace map_pointer_t with map_pointer This is a first step to a type safe cache, adding typing information everywhere. Next, we'll replace map_pointer implementation with a type safe one. --- apt-pkg/pkgcachegen.cc | 72 +++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 36 deletions(-) (limited to 'apt-pkg/pkgcachegen.cc') diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index 5a7272e84..7a6ada5a2 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -77,7 +77,7 @@ bool pkgCacheGenerator::Start() *Cache.HeaderP = pkgCache::Header(); // make room for the hashtables for packages and groups - if (Map.RawAllocate(2 * (Cache.HeaderP->GetHashTableSize() * sizeof(map_pointer_t))) == 0) + if (Map.RawAllocate(2 * (Cache.HeaderP->GetHashTableSize() * sizeof(map_pointer))) == 0) return false; map_stringitem_t const idxVerSysName = WriteStringInMap(_system->VS->Label); @@ -226,10 +226,10 @@ map_stringitem_t pkgCacheGenerator::WriteStringInMap(const char *String) { return index; } /*}}}*/ -map_pointer_t pkgCacheGenerator::AllocateInMap(const unsigned long &size) {/*{{{*/ +map_pointer pkgCacheGenerator::AllocateInMap(const unsigned long &size) {/*{{{*/ size_t oldSize = Map.Size(); void const * const oldMap = Map.Data(); - map_pointer_t const index = Map.Allocate(size); + map_pointer const index = Map.Allocate(size); if (index != 0) ReMap(oldMap, Map.Data(), oldSize); return index; @@ -373,7 +373,7 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator { pkgCache::VerIterator Ver = Pkg.VersionList(); Dynamic DynVer(Ver); - map_pointer_t *LastVer = &Pkg->VersionList; + map_pointer *LastVer = &Pkg->VersionList; void const * oldMap = Map.Data(); auto Hash = List.VersionHash(); @@ -433,13 +433,13 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator } // Add a new version - map_pointer_t const verindex = NewVersion(Ver, Version, Pkg.Index(), Hash, *LastVer); + map_pointer const verindex = NewVersion(Ver, Version, Pkg.Index(), Hash, *LastVer); if (unlikely(verindex == 0)) return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "NewVersion", 1); if (oldMap != Map.Data()) - LastVer += static_cast(Map.Data()) - static_cast(oldMap); + LastVer += static_cast const *>(Map.Data()) - static_cast const *>(oldMap); *LastVer = verindex; if (unlikely(List.NewVersion(Ver) == false)) @@ -519,7 +519,7 @@ bool pkgCacheGenerator::AddNewDescription(ListParser &List, pkgCache::VerIterato pkgCache::DescIterator Desc; Dynamic DynDesc(Desc); - map_pointer_t const descindex = NewDescription(Desc, lang, CurMd5, md5idx); + map_pointer const descindex = NewDescription(Desc, lang, CurMd5, md5idx); if (unlikely(descindex == 0)) return _error->Error(_("Error occurred while processing %s (%s%d)"), Ver.ParentPkg().Name(), "NewDescription", 1); @@ -531,7 +531,7 @@ bool pkgCacheGenerator::AddNewDescription(ListParser &List, pkgCache::VerIterato // that to be able to efficiently share these lists pkgCache::DescIterator VerDesc = Ver.DescriptionList(); // old value might be invalid after ReMap for (;VerDesc.end() == false && VerDesc->NextDesc != 0; ++VerDesc); - map_pointer_t * const LastNextDesc = (VerDesc.end() == true) ? &Ver->DescriptionList : &VerDesc->NextDesc; + map_pointer * const LastNextDesc = (VerDesc.end() == true) ? &Ver->DescriptionList : &VerDesc->NextDesc; *LastNextDesc = descindex; if (NewFileDesc(Desc,List) == false) @@ -553,7 +553,7 @@ bool pkgCacheGenerator::NewGroup(pkgCache::GrpIterator &Grp, StringView Name) return true; // Get a structure - map_pointer_t const Group = AllocateInMap(sizeof(pkgCache::Group)); + map_pointer const Group = AllocateInMap(sizeof(pkgCache::Group)); if (unlikely(Group == 0)) return false; @@ -566,7 +566,7 @@ bool pkgCacheGenerator::NewGroup(pkgCache::GrpIterator &Grp, StringView Name) // Insert it into the hash table unsigned long const Hash = Cache.Hash(Name); - map_pointer_t *insertAt = &Cache.HeaderP->GrpHashTableP()[Hash]; + map_pointer *insertAt = &Cache.HeaderP->GrpHashTableP()[Hash]; while (*insertAt != 0 && StringViewCompareFast(Name, Cache.ViewString((Cache.GrpP + *insertAt)->Name)) > 0) insertAt = &(Cache.GrpP + *insertAt)->Next; @@ -594,7 +594,7 @@ bool pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, StringView Name, return true; // Get a structure - map_pointer_t const Package = AllocateInMap(sizeof(pkgCache::Package)); + map_pointer const Package = AllocateInMap(sizeof(pkgCache::Package)); if (unlikely(Package == 0)) return false; Pkg = pkgCache::PkgIterator(Cache,Cache.PkgP + Package); @@ -614,7 +614,7 @@ bool pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, StringView Name, Grp->FirstPackage = Package; // Insert it into the hash table map_id_t const Hash = Cache.Hash(Name); - map_pointer_t *insertAt = &Cache.HeaderP->PkgHashTableP()[Hash]; + map_pointer *insertAt = &Cache.HeaderP->PkgHashTableP()[Hash]; while (*insertAt != 0 && StringViewCompareFast(Name, Cache.ViewString((Cache.GrpP + (Cache.PkgP + *insertAt)->Group)->Name)) > 0) insertAt = &(Cache.PkgP + *insertAt)->NextPackage; Pkg->NextPackage = *insertAt; @@ -678,7 +678,7 @@ bool pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, StringView Name, continue; pkgCache::VerIterator Ver = Dep.ParentVer(); Dynamic DynVer(Ver); - map_pointer_t * unused = NULL; + map_pointer * unused = NULL; if (NewDepends(Pkg, Ver, Dep->Version, Dep->CompareOp, Dep->Type, unused) == false) return false; } @@ -747,7 +747,7 @@ bool pkgCacheGenerator::AddImplicitDepends(pkgCache::GrpIterator &G, { APT::StringView Arch = P.Arch() == NULL ? "" : P.Arch(); Dynamic DynArch(Arch); - map_pointer_t *OldDepLast = NULL; + map_pointer *OldDepLast = NULL; /* MultiArch handling introduces a lot of implicit Dependencies: - MultiArch: same → Co-Installable if they have the same version - All others conflict with all other group members */ @@ -787,7 +787,7 @@ bool pkgCacheGenerator::AddImplicitDepends(pkgCache::VerIterator &V, /* MultiArch handling introduces a lot of implicit Dependencies: - MultiArch: same → Co-Installable if they have the same version - All others conflict with all other group members */ - map_pointer_t *OldDepLast = NULL; + map_pointer *OldDepLast = NULL; bool const coInstall = ((V->MultiArch & pkgCache::Version::Same) == pkgCache::Version::Same); if (coInstall == true) { @@ -820,7 +820,7 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, return true; // Get a structure - map_pointer_t const VerFile = AllocateInMap(sizeof(pkgCache::VerFile)); + map_pointer const VerFile = AllocateInMap(sizeof(pkgCache::VerFile)); if (VerFile == 0) return false; @@ -828,7 +828,7 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, VF->File = CurrentFile - Cache.PkgFileP; // Link it to the end of the list - map_pointer_t *Last = &Ver->FileList; + map_pointer *Last = &Ver->FileList; for (pkgCache::VerFileIterator V = Ver.FileList(); V.end() == false; ++V) Last = &V->NextFile; VF->NextFile = *Last; @@ -846,14 +846,14 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, // CacheGenerator::NewVersion - Create a new Version /*{{{*/ // --------------------------------------------------------------------- /* This puts a version structure in the linked list */ -map_pointer_t pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, +map_pointer pkgCacheGenerator::NewVersion(pkgCache::VerIterator &Ver, APT::StringView const &VerStr, - map_pointer_t const ParentPkg, + map_pointer const ParentPkg, uint32_t Hash, - map_pointer_t const Next) + map_pointer const Next) { // Get a structure - map_pointer_t const Version = AllocateInMap(sizeof(pkgCache::Version)); + map_pointer const Version = AllocateInMap(sizeof(pkgCache::Version)); if (Version == 0) return 0; @@ -905,7 +905,7 @@ bool pkgCacheGenerator::NewFileDesc(pkgCache::DescIterator &Desc, return true; // Get a structure - map_pointer_t const DescFile = AllocateInMap(sizeof(pkgCache::DescFile)); + map_pointer const DescFile = AllocateInMap(sizeof(pkgCache::DescFile)); if (DescFile == 0) return false; @@ -913,7 +913,7 @@ bool pkgCacheGenerator::NewFileDesc(pkgCache::DescIterator &Desc, DF->File = CurrentFile - Cache.PkgFileP; // Link it to the end of the list - map_pointer_t *Last = &Desc->FileList; + map_pointer *Last = &Desc->FileList; for (pkgCache::DescFileIterator D = Desc.FileList(); D.end() == false; ++D) Last = &D->NextFile; @@ -932,13 +932,13 @@ bool pkgCacheGenerator::NewFileDesc(pkgCache::DescIterator &Desc, // CacheGenerator::NewDescription - Create a new Description /*{{{*/ // --------------------------------------------------------------------- /* This puts a description structure in the linked list */ -map_pointer_t pkgCacheGenerator::NewDescription(pkgCache::DescIterator &Desc, +map_pointer pkgCacheGenerator::NewDescription(pkgCache::DescIterator &Desc, const string &Lang, APT::StringView md5sum, map_stringitem_t const idxmd5str) { // Get a structure - map_pointer_t const Description = AllocateInMap(sizeof(pkgCache::Description)); + map_pointer const Description = AllocateInMap(sizeof(pkgCache::Description)); if (Description == 0) return 0; @@ -969,20 +969,20 @@ map_pointer_t pkgCacheGenerator::NewDescription(pkgCache::DescIterator &Desc, version and to the package that it is pointing to. */ bool pkgCacheGenerator::NewDepends(pkgCache::PkgIterator &Pkg, pkgCache::VerIterator &Ver, - map_pointer_t const Version, + map_pointer const Version, uint8_t const Op, uint8_t const Type, - map_pointer_t* &OldDepLast) + map_pointer * &OldDepLast) { void const * const oldMap = Map.Data(); // Get a structure - map_pointer_t const Dependency = AllocateInMap(sizeof(pkgCache::Dependency)); + map_pointer const Dependency = AllocateInMap(sizeof(pkgCache::Dependency)); if (unlikely(Dependency == 0)) return false; bool isDuplicate = false; - map_pointer_t DependencyData = 0; - map_pointer_t PreviousData = 0; + map_pointer DependencyData = 0; + map_pointer PreviousData = 0; if (Pkg->RevDepends != 0) { pkgCache::Dependency const * const L = Cache.DepP + Pkg->RevDepends; @@ -1054,7 +1054,7 @@ bool pkgCacheGenerator::NewDepends(pkgCache::PkgIterator &Pkg, for (pkgCache::DepIterator D = Ver.DependsList(); D.end() == false; ++D) OldDepLast = &D->NextDepends; } else if (oldMap != Map.Data()) - OldDepLast += static_cast(Map.Data()) - static_cast(oldMap); + OldDepLast += static_cast const *>(Map.Data()) - static_cast const *>(oldMap); Dep->NextDepends = *OldDepLast; *OldDepLast = Dependency; @@ -1171,11 +1171,11 @@ bool pkgCacheListParser::NewProvides(pkgCache::VerIterator &Ver, } bool pkgCacheGenerator::NewProvides(pkgCache::VerIterator &Ver, pkgCache::PkgIterator &Pkg, - map_pointer_t const ProvideVersion, + map_stringitem_t const ProvideVersion, uint8_t const Flags) { // Get a structure - map_pointer_t const Provides = AllocateInMap(sizeof(pkgCache::Provides)); + map_pointer const Provides = AllocateInMap(sizeof(pkgCache::Provides)); if (unlikely(Provides == 0)) return false; ++Cache.HeaderP->ProvidesCount; @@ -1249,7 +1249,7 @@ bool pkgCacheGenerator::SelectReleaseFile(const string &File,const string &Site, return true; // Get some space for the structure - map_pointer_t const idxFile = AllocateInMap(sizeof(*CurrentRlsFile)); + map_pointer const idxFile = AllocateInMap(sizeof(*CurrentRlsFile)); if (unlikely(idxFile == 0)) return false; CurrentRlsFile = Cache.RlsFileP + idxFile; @@ -1283,7 +1283,7 @@ bool pkgCacheGenerator::SelectFile(std::string const &File, { CurrentFile = nullptr; // Get some space for the structure - map_pointer_t const idxFile = AllocateInMap(sizeof(*CurrentFile)); + map_pointer const idxFile = AllocateInMap(sizeof(*CurrentFile)); if (unlikely(idxFile == 0)) return false; CurrentFile = Cache.PkgFileP + idxFile; @@ -1630,7 +1630,7 @@ static bool loadBackMMapFromFile(std::unique_ptr &Gen, if (CacheF.IsOpen() == false || CacheF.Seek(0) == false || CacheF.Failed()) return false; _error->PushToStack(); - map_pointer_t const alloc = Map->RawAllocate(CacheF.Size()); + map_pointer const alloc = Map->RawAllocate(CacheF.Size()); bool const newError = _error->PendingError(); _error->MergeWithStack(); if (alloc == 0 && newError) -- cgit v1.2.3-70-g09d2 From 1f4e2ab7462f5e05e452fb8505185895d91651c2 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 24 Feb 2020 18:09:49 +0100 Subject: Wrap AllocateInMap with a templated version --- apt-pkg/pkgcachegen.cc | 30 ++++++++++++++++-------------- apt-pkg/pkgcachegen.h | 5 ++++- 2 files changed, 20 insertions(+), 15 deletions(-) (limited to 'apt-pkg/pkgcachegen.cc') diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index 7a6ada5a2..a1e6ca745 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -226,13 +226,15 @@ map_stringitem_t pkgCacheGenerator::WriteStringInMap(const char *String) { return index; } /*}}}*/ -map_pointer pkgCacheGenerator::AllocateInMap(const unsigned long &size) {/*{{{*/ +uint32_t pkgCacheGenerator::AllocateInMap(const unsigned long &size) {/*{{{*/ size_t oldSize = Map.Size(); void const * const oldMap = Map.Data(); - map_pointer const index = Map.Allocate(size); + auto index = Map.Allocate(size); if (index != 0) ReMap(oldMap, Map.Data(), oldSize); - return index; + if (index != static_cast(index)) + abort(); // Internal error + return static_cast(index); } /*}}}*/ // CacheGenerator::MergeList - Merge the package list /*{{{*/ @@ -553,7 +555,7 @@ bool pkgCacheGenerator::NewGroup(pkgCache::GrpIterator &Grp, StringView Name) return true; // Get a structure - map_pointer const Group = AllocateInMap(sizeof(pkgCache::Group)); + auto const Group = AllocateInMap(); if (unlikely(Group == 0)) return false; @@ -594,7 +596,7 @@ bool pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, StringView Name, return true; // Get a structure - map_pointer const Package = AllocateInMap(sizeof(pkgCache::Package)); + auto const Package = AllocateInMap(); if (unlikely(Package == 0)) return false; Pkg = pkgCache::PkgIterator(Cache,Cache.PkgP + Package); @@ -820,7 +822,7 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, return true; // Get a structure - map_pointer const VerFile = AllocateInMap(sizeof(pkgCache::VerFile)); + auto const VerFile = AllocateInMap(); if (VerFile == 0) return false; @@ -853,7 +855,7 @@ map_pointer pkgCacheGenerator::NewVersion(pkgCache::VerIterat map_pointer const Next) { // Get a structure - map_pointer const Version = AllocateInMap(sizeof(pkgCache::Version)); + auto const Version = AllocateInMap(); if (Version == 0) return 0; @@ -905,7 +907,7 @@ bool pkgCacheGenerator::NewFileDesc(pkgCache::DescIterator &Desc, return true; // Get a structure - map_pointer const DescFile = AllocateInMap(sizeof(pkgCache::DescFile)); + auto const DescFile = AllocateInMap(); if (DescFile == 0) return false; @@ -938,7 +940,7 @@ map_pointer pkgCacheGenerator::NewDescription(pkgCache::D map_stringitem_t const idxmd5str) { // Get a structure - map_pointer const Description = AllocateInMap(sizeof(pkgCache::Description)); + auto const Description = AllocateInMap(); if (Description == 0) return 0; @@ -976,7 +978,7 @@ bool pkgCacheGenerator::NewDepends(pkgCache::PkgIterator &Pkg, { void const * const oldMap = Map.Data(); // Get a structure - map_pointer const Dependency = AllocateInMap(sizeof(pkgCache::Dependency)); + auto const Dependency = AllocateInMap(); if (unlikely(Dependency == 0)) return false; @@ -1003,7 +1005,7 @@ bool pkgCacheGenerator::NewDepends(pkgCache::PkgIterator &Pkg, if (isDuplicate == false) { - DependencyData = AllocateInMap(sizeof(pkgCache::DependencyData)); + DependencyData = AllocateInMap(); if (unlikely(DependencyData == 0)) return false; } @@ -1175,7 +1177,7 @@ bool pkgCacheGenerator::NewProvides(pkgCache::VerIterator &Ver, uint8_t const Flags) { // Get a structure - map_pointer const Provides = AllocateInMap(sizeof(pkgCache::Provides)); + auto const Provides = AllocateInMap(); if (unlikely(Provides == 0)) return false; ++Cache.HeaderP->ProvidesCount; @@ -1249,7 +1251,7 @@ bool pkgCacheGenerator::SelectReleaseFile(const string &File,const string &Site, return true; // Get some space for the structure - map_pointer const idxFile = AllocateInMap(sizeof(*CurrentRlsFile)); + auto const idxFile = AllocateInMap(); if (unlikely(idxFile == 0)) return false; CurrentRlsFile = Cache.RlsFileP + idxFile; @@ -1283,7 +1285,7 @@ bool pkgCacheGenerator::SelectFile(std::string const &File, { CurrentFile = nullptr; // Get some space for the structure - map_pointer const idxFile = AllocateInMap(sizeof(*CurrentFile)); + auto const idxFile = AllocateInMap(); if (unlikely(idxFile == 0)) return false; CurrentFile = Cache.PkgFileP + idxFile; diff --git a/apt-pkg/pkgcachegen.h b/apt-pkg/pkgcachegen.h index d088bca52..c042625c4 100644 --- a/apt-pkg/pkgcachegen.h +++ b/apt-pkg/pkgcachegen.h @@ -40,7 +40,10 @@ class APT_HIDDEN pkgCacheGenerator /*{{{*/ APT_HIDDEN map_stringitem_t WriteStringInMap(APT::StringView String) { return WriteStringInMap(String.data(), String.size()); }; APT_HIDDEN map_stringitem_t WriteStringInMap(const char *String); APT_HIDDEN map_stringitem_t WriteStringInMap(const char *String, const unsigned long &Len); - APT_HIDDEN map_pointer AllocateInMap(const unsigned long &size); + APT_HIDDEN uint32_t AllocateInMap(const unsigned long &size); + template map_pointer AllocateInMap() { + return map_pointer{AllocateInMap(sizeof(T))}; + } // Dirty hack for public users that do not use C++11 yet #if __cplusplus >= 201103L -- cgit v1.2.3-70-g09d2 From 4fad7262291a8af1415fb9a3693678bd9610f0d6 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Mon, 24 Feb 2020 17:46:10 +0100 Subject: Make map_pointer typesafe Instead of just using uint32_t, which would allow you to assign e.g. a map_pointer to a map_pointer, use our own smarter struct that has strict type checking. We allow creating a map_pointer from a nullptr, and we allow comparing map_pointer to nullptr, which also deals with comparisons against 0 which are often used, as 0 will be implictly converted to nullptr. --- apt-pkg/cacheiterators.h | 9 +++++---- apt-pkg/deb/deblistparser.cc | 4 ++-- apt-pkg/edsp/edsplistparser.cc | 4 ++-- apt-pkg/pkgcache.cc | 2 +- apt-pkg/pkgcache.h | 22 +++++++++++++++++++- apt-pkg/pkgcachegen.cc | 43 ++++++++++++++++++++-------------------- apt-pkg/pkgcachegen.h | 2 +- apt-private/private-cachefile.cc | 2 +- cmdline/apt-cache.cc | 4 ++-- 9 files changed, 57 insertions(+), 35 deletions(-) (limited to 'apt-pkg/pkgcachegen.cc') diff --git a/apt-pkg/cacheiterators.h b/apt-pkg/cacheiterators.h index 7f853558b..d2e4f7f90 100644 --- a/apt-pkg/cacheiterators.h +++ b/apt-pkg/cacheiterators.h @@ -80,6 +80,7 @@ template class pkgCache::Iterator : // Mixed stuff inline bool IsGood() const { return S && Owner && ! end();} inline unsigned long Index() const {return S - OwnerPointer();} + inline map_pointer MapPointer() const {return map_pointer(Index()) ;} void ReMap(void const * const oldMap, void const * const newMap) { if (Owner == 0 || S == 0) @@ -293,7 +294,7 @@ class pkgCache::DepIterator : public Iterator { inline PkgIterator TargetPkg() const {return PkgIterator(*Owner,Owner->PkgP + S2->Package);} inline PkgIterator SmartTargetPkg() const {PkgIterator R(*Owner,0);SmartTargetPkg(R);return R;} inline VerIterator ParentVer() const {return VerIterator(*Owner,Owner->VerP + S->ParentVer);} - inline PkgIterator ParentPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[S->ParentVer].ParentPkg);} + inline PkgIterator ParentPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[uint32_t(S->ParentVer)].ParentPkg);} inline bool Reverse() const {return Type == DepRev;} bool IsCritical() const APT_PURE; bool IsNegative() const APT_PURE; @@ -378,7 +379,7 @@ class pkgCache::PrvIterator : public Iterator { inline const char *ProvideVersion() const {return S->ProvideVersion == 0?0:Owner->StrP + S->ProvideVersion;} inline PkgIterator ParentPkg() const {return PkgIterator(*Owner,Owner->PkgP + S->ParentPkg);} inline VerIterator OwnerVer() const {return VerIterator(*Owner,Owner->VerP + S->Version);} - inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[S->Version].ParentPkg);} + inline PkgIterator OwnerPkg() const {return PkgIterator(*Owner,Owner->PkgP + Owner->VerP[uint32_t(S->Version)].ParentPkg);} /* MultiArch can be translated to SingleArch for an resolver and we did so, by adding provides to help the resolver understand the problem, but @@ -475,7 +476,7 @@ class pkgCache::VerFileIterator : public pkgCache::IteratorFile + Owner->PkgFileP);} + inline PkgFileIterator File() const {return PkgFileIterator(*Owner, Owner->PkgFileP + S->File);} inline VerFileIterator() : Iterator() {} inline VerFileIterator(pkgCache &Owner,VerFile *Trg) : Iterator(Owner, Trg) {} @@ -493,7 +494,7 @@ class pkgCache::DescFileIterator : public Iterator { inline DescFileIterator operator++(int) { DescFileIterator const tmp(*this); operator++(); return tmp; } // Accessors - inline PkgFileIterator File() const {return PkgFileIterator(*Owner,S->File + Owner->PkgFileP);} + inline PkgFileIterator File() const {return PkgFileIterator(*Owner, Owner->PkgFileP + S->File);} inline DescFileIterator() : Iterator() {} inline DescFileIterator(pkgCache &Owner,DescFile *Trg) : Iterator(Owner, Trg) {} diff --git a/apt-pkg/deb/deblistparser.cc b/apt-pkg/deb/deblistparser.cc index eaa9dfda9..ab957a01a 100644 --- a/apt-pkg/deb/deblistparser.cc +++ b/apt-pkg/deb/deblistparser.cc @@ -208,7 +208,7 @@ bool debListParser::NewVersion(pkgCache::VerIterator &Ver) // Link into by source package group. Ver->SourcePkgName = G->Name; Ver->NextInSource = G->VersionsInSource; - G->VersionsInSource = Ver.Index(); + G->VersionsInSource = Ver.MapPointer(); Ver->MultiArch = ParseMultiArch(true); // Archive Size @@ -469,7 +469,7 @@ bool debStatusListParser::ParseStatus(pkgCache::PkgIterator &Pkg, if (Ver.end() == true) _error->Warning("Encountered status field in a non-version description"); else - Pkg->CurrentVer = Ver.Index(); + Pkg->CurrentVer = Ver.MapPointer(); } return true; diff --git a/apt-pkg/edsp/edsplistparser.cc b/apt-pkg/edsp/edsplistparser.cc index 45abdbc61..34b9ec934 100644 --- a/apt-pkg/edsp/edsplistparser.cc +++ b/apt-pkg/edsp/edsplistparser.cc @@ -87,7 +87,7 @@ bool edspListParser::ParseStatus(pkgCache::PkgIterator &Pkg, if (state != 0) { Pkg->CurrentState = pkgCache::State::Installed; - Pkg->CurrentVer = Ver.Index(); + Pkg->CurrentVer = Ver.MapPointer(); } if (Section.FindB("APT-Automatic", false)) @@ -162,7 +162,7 @@ bool eippListParser::ParseStatus(pkgCache::PkgIterator &Pkg, case pkgCache::State::TriggersAwaited: case pkgCache::State::TriggersPending: case pkgCache::State::Installed: - Pkg->CurrentVer = Ver.Index(); + Pkg->CurrentVer = Ver.MapPointer(); break; } break; diff --git a/apt-pkg/pkgcache.cc b/apt-pkg/pkgcache.cc index 59f4256ea..02448a073 100644 --- a/apt-pkg/pkgcache.cc +++ b/apt-pkg/pkgcache.cc @@ -451,7 +451,7 @@ pkgCache::PkgIterator pkgCache::GrpIterator::NextPkg(pkgCache::PkgIterator const LastPkg.end() == true)) return PkgIterator(*Owner, 0); - if (S->LastPackage == LastPkg.Index()) + if (S->LastPackage == LastPkg.MapPointer()) return PkgIterator(*Owner, 0); return PkgIterator(*Owner, Owner->PkgP + LastPkg->NextPackage); diff --git a/apt-pkg/pkgcache.h b/apt-pkg/pkgcache.h index e5a1a81eb..ef22a78a0 100644 --- a/apt-pkg/pkgcache.h +++ b/apt-pkg/pkgcache.h @@ -77,6 +77,7 @@ #include #include +#include // required for nullptr_t #include #include #include @@ -92,10 +93,29 @@ typedef uint32_t map_filesize_small_t; typedef uint32_t map_id_t; // some files get an id, too, but in far less absolute numbers typedef uint16_t map_fileid_t; + // relative pointer from cache start -template using map_pointer = uint32_t; +template class map_pointer { + uint32_t val; +public: + map_pointer() noexcept : val(0) {} + map_pointer(nullptr_t) noexcept : val(0) {} + explicit map_pointer(uint32_t n) noexcept : val(n) {} + explicit operator uint32_t() noexcept { return val; } +}; + +template inline T *operator +(T *p, map_pointer m) { return p + uint32_t(m); } +template inline bool operator ==(map_pointer u, map_pointer m) { return uint32_t(u) == uint32_t(m); } +template inline bool operator !=(map_pointer u, map_pointer m) { return uint32_t(u) != uint32_t(m); } +template inline bool operator <(map_pointer u, map_pointer m) { return uint32_t(u) < uint32_t(m); } +template inline bool operator >(map_pointer u, map_pointer m) { return uint32_t(u) > uint32_t(m); } +template inline uint32_t operator -(map_pointer u, map_pointer m) { return uint32_t(u) - uint32_t(m); } +template bool operator ==(map_pointer m, nullptr_t) { return uint32_t(m) == 0; } +template bool operator !=(map_pointer m, nullptr_t) { return uint32_t(m) != 0; } + // same as the previous, but documented to be to a string item typedef map_pointer map_stringitem_t; + // we have only a small amount of flags for each item typedef uint8_t map_flags_t; typedef uint8_t map_number_t; diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index a1e6ca745..83a8c2e63 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -210,7 +210,7 @@ map_stringitem_t pkgCacheGenerator::WriteStringInMap(const char *String, const unsigned long &Len) { size_t oldSize = Map.Size(); void const * const oldMap = Map.Data(); - map_stringitem_t const index = Map.WriteString(String, Len); + map_stringitem_t const index{Map.WriteString(String, Len)}; if (index != 0) ReMap(oldMap, Map.Data(), oldSize); return index; @@ -220,7 +220,7 @@ map_stringitem_t pkgCacheGenerator::WriteStringInMap(const char *String, map_stringitem_t pkgCacheGenerator::WriteStringInMap(const char *String) { size_t oldSize = Map.Size(); void const * const oldMap = Map.Data(); - map_stringitem_t const index = Map.WriteString(String); + map_stringitem_t const index{Map.WriteString(String)}; if (index != 0) ReMap(oldMap, Map.Data(), oldSize); return index; @@ -375,7 +375,7 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator { pkgCache::VerIterator Ver = Pkg.VersionList(); Dynamic DynVer(Ver); - map_pointer *LastVer = &Pkg->VersionList; + map_pointer *LastVer = &Pkg->VersionList; void const * oldMap = Map.Data(); auto Hash = List.VersionHash(); @@ -435,7 +435,7 @@ bool pkgCacheGenerator::MergeListVersion(ListParser &List, pkgCache::PkgIterator } // Add a new version - map_pointer const verindex = NewVersion(Ver, Version, Pkg.Index(), Hash, *LastVer); + map_pointer const verindex = NewVersion(Ver, Version, Pkg.MapPointer(), Hash, *LastVer); if (unlikely(verindex == 0)) return _error->Error(_("Error occurred while processing %s (%s%d)"), Pkg.Name(), "NewVersion", 1); @@ -527,7 +527,7 @@ bool pkgCacheGenerator::AddNewDescription(ListParser &List, pkgCache::VerIterato Ver.ParentPkg().Name(), "NewDescription", 1); md5idx = Desc->md5sum; - Desc->ParentPkg = Ver.ParentPkg().Index(); + Desc->ParentPkg = Ver.ParentPkg().MapPointer(); // we add at the end, so that the start is constant as we need // that to be able to efficiently share these lists @@ -602,7 +602,7 @@ bool pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, StringView Name, Pkg = pkgCache::PkgIterator(Cache,Cache.PkgP + Package); // Set the name, arch and the ID - Pkg->Group = Grp.Index(); + Pkg->Group = Grp.MapPointer(); // all is mapped to the native architecture map_stringitem_t const idxArch = (Arch == "all") ? Cache.HeaderP->Architecture : StoreString(MIXED, Arch); if (unlikely(idxArch == 0)) @@ -827,14 +827,14 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, return false; pkgCache::VerFileIterator VF(Cache,Cache.VerFileP + VerFile); - VF->File = CurrentFile - Cache.PkgFileP; + VF->File = map_pointer{CurrentFile - Cache.PkgFileP}; // Link it to the end of the list map_pointer *Last = &Ver->FileList; for (pkgCache::VerFileIterator V = Ver.FileList(); V.end() == false; ++V) Last = &V->NextFile; VF->NextFile = *Last; - *Last = VF.Index(); + *Last = VF.MapPointer(); VF->Offset = List.Offset(); VF->Size = List.Size(); @@ -912,7 +912,7 @@ bool pkgCacheGenerator::NewFileDesc(pkgCache::DescIterator &Desc, return false; pkgCache::DescFileIterator DF(Cache,Cache.DescFileP + DescFile); - DF->File = CurrentFile - Cache.PkgFileP; + DF->File = map_pointer{CurrentFile - Cache.PkgFileP}; // Link it to the end of the list map_pointer *Last = &Desc->FileList; @@ -920,7 +920,7 @@ bool pkgCacheGenerator::NewFileDesc(pkgCache::DescIterator &Desc, Last = &D->NextFile; DF->NextFile = *Last; - *Last = DF.Index(); + *Last = DF.MapPointer(); DF->Offset = List.Offset(); DF->Size = List.Size(); @@ -971,7 +971,7 @@ map_pointer pkgCacheGenerator::NewDescription(pkgCache::D version and to the package that it is pointing to. */ bool pkgCacheGenerator::NewDepends(pkgCache::PkgIterator &Pkg, pkgCache::VerIterator &Ver, - map_pointer const Version, + map_stringitem_t const Version, uint8_t const Op, uint8_t const Type, map_pointer * &OldDepLast) @@ -1011,7 +1011,7 @@ bool pkgCacheGenerator::NewDepends(pkgCache::PkgIterator &Pkg, } pkgCache::Dependency * Link = Cache.DepP + Dependency; - Link->ParentVer = Ver.Index(); + Link->ParentVer = Ver.MapPointer(); Link->DependencyData = DependencyData; Link->ID = Cache.HeaderP->DependsCount++; @@ -1021,7 +1021,7 @@ bool pkgCacheGenerator::NewDepends(pkgCache::PkgIterator &Pkg, Dep->Type = Type; Dep->CompareOp = Op; Dep->Version = Version; - Dep->Package = Pkg.Index(); + Dep->Package = Pkg.MapPointer(); ++Cache.HeaderP->DependsDataCount; if (PreviousData != 0) { @@ -1184,16 +1184,16 @@ bool pkgCacheGenerator::NewProvides(pkgCache::VerIterator &Ver, // Fill it in pkgCache::PrvIterator Prv(Cache,Cache.ProvideP + Provides,Cache.PkgP); - Prv->Version = Ver.Index(); + Prv->Version = Ver.MapPointer(); Prv->ProvideVersion = ProvideVersion; Prv->Flags = Flags; Prv->NextPkgProv = Ver->ProvidesList; - Ver->ProvidesList = Prv.Index(); + Ver->ProvidesList = Prv.MapPointer(); // Link it to the package - Prv->ParentPkg = Pkg.Index(); + Prv->ParentPkg = Pkg.MapPointer(); Prv->NextProvides = Pkg->ProvidesList; - Pkg->ProvidesList = Prv.Index(); + Pkg->ProvidesList = Prv.MapPointer(); return true; } /*}}}*/ @@ -1267,7 +1267,7 @@ bool pkgCacheGenerator::SelectReleaseFile(const string &File,const string &Site, CurrentRlsFile->Flags = Flags; CurrentRlsFile->ID = Cache.HeaderP->ReleaseFileCount; RlsFileName = File; - Cache.HeaderP->RlsFileList = CurrentRlsFile - Cache.RlsFileP; + Cache.HeaderP->RlsFileList = map_pointer{CurrentRlsFile - Cache.RlsFileP}; Cache.HeaderP->ReleaseFileCount++; return true; @@ -1316,11 +1316,11 @@ bool pkgCacheGenerator::SelectFile(std::string const &File, CurrentFile->Component = component; CurrentFile->Flags = Flags; if (CurrentRlsFile != nullptr) - CurrentFile->Release = CurrentRlsFile - Cache.RlsFileP; + CurrentFile->Release = map_pointer{CurrentRlsFile - Cache.RlsFileP}; else CurrentFile->Release = 0; PkgFileName = File; - Cache.HeaderP->FileList = CurrentFile - Cache.PkgFileP; + Cache.HeaderP->FileList = map_pointer{CurrentFile - Cache.PkgFileP}; Cache.HeaderP->PackageFileCount++; if (Progress != 0) @@ -1362,6 +1362,7 @@ public: ScopedErrorRevert() { _error->PushToStack(); } ~ScopedErrorRevert() { _error->RevertToStack(); } }; + static bool CheckValidity(FileFd &CacheFile, std::string const &CacheFileName, pkgSourceList &List, FileIterator const Start, @@ -1632,7 +1633,7 @@ static bool loadBackMMapFromFile(std::unique_ptr &Gen, if (CacheF.IsOpen() == false || CacheF.Seek(0) == false || CacheF.Failed()) return false; _error->PushToStack(); - map_pointer const alloc = Map->RawAllocate(CacheF.Size()); + uint32_t const alloc = Map->RawAllocate(CacheF.Size()); bool const newError = _error->PendingError(); _error->MergeWithStack(); if (alloc == 0 && newError) diff --git a/apt-pkg/pkgcachegen.h b/apt-pkg/pkgcachegen.h index c042625c4..f5b4c80b3 100644 --- a/apt-pkg/pkgcachegen.h +++ b/apt-pkg/pkgcachegen.h @@ -118,7 +118,7 @@ class APT_HIDDEN pkgCacheGenerator /*{{{*/ bool NewFileVer(pkgCache::VerIterator &Ver,ListParser &List); bool NewFileDesc(pkgCache::DescIterator &Desc,ListParser &List); bool NewDepends(pkgCache::PkgIterator &Pkg, pkgCache::VerIterator &Ver, - map_pointer const Version, uint8_t const Op, + map_stringitem_t const Version, uint8_t const Op, uint8_t const Type, map_pointer* &OldDepLast); bool NewProvides(pkgCache::VerIterator &Ver, pkgCache::PkgIterator &Pkg, map_stringitem_t const ProvidesVersion, uint8_t const Flags); diff --git a/apt-private/private-cachefile.cc b/apt-private/private-cachefile.cc index 4becc147e..9d875b4a7 100644 --- a/apt-private/private-cachefile.cc +++ b/apt-private/private-cachefile.cc @@ -22,7 +22,7 @@ using namespace std; static bool SortPackagesByName(pkgCache * const Owner, - map_pointer const A, map_pointer const B) + map_pointer const A, map_pointer const B) { if (A == 0) return false; diff --git a/cmdline/apt-cache.cc b/cmdline/apt-cache.cc index 2d6c1a91e..23ab7e47f 100644 --- a/cmdline/apt-cache.cc +++ b/cmdline/apt-cache.cc @@ -467,7 +467,7 @@ static bool DumpAvail(CommandLine &) char *Buffer = new char[Cache->HeaderP->MaxVerFileSize+10]; for (pkgCache::VerFile **J = VFList; *J != 0;) { - pkgCache::PkgFileIterator File(*Cache,(*J)->File + Cache->PkgFileP); + pkgCache::PkgFileIterator File(*Cache, Cache->PkgFileP + (*J)->File); // FIXME: Add support for volatile/with-source files FileFd PkgF(File.FileName(),FileFd::ReadOnly, FileFd::Extension); if (_error->PendingError() == true) @@ -481,7 +481,7 @@ static bool DumpAvail(CommandLine &) unsigned long Pos = 0; for (; *J != 0; J++) { - if ((*J)->File + Cache->PkgFileP != File) + if (Cache->PkgFileP + (*J)->File != File) break; const pkgCache::VerFile &VF = **J; -- cgit v1.2.3-70-g09d2 From f17e3ce8213ba364d1eea7b5bab286a2175e5875 Mon Sep 17 00:00:00 2001 From: Julian Andres Klode Date: Tue, 25 Feb 2020 11:26:18 +0100 Subject: Silence narrow conversion warnings, add error checks When converting a long offset to a uint32_t to be stored in the map, check that this is safe to do. If the offset is negative, or we lose data in the conversion, we lost. --- apt-pkg/pkgcachegen.cc | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) (limited to 'apt-pkg/pkgcachegen.cc') diff --git a/apt-pkg/pkgcachegen.cc b/apt-pkg/pkgcachegen.cc index 83a8c2e63..283866f88 100644 --- a/apt-pkg/pkgcachegen.cc +++ b/apt-pkg/pkgcachegen.cc @@ -48,6 +48,19 @@ static bool IsDuplicateDescription(pkgCache &Cache, pkgCache::DescIterator Desc, using std::string; using APT::StringView; +// Convert an offset returned from e.g. DynamicMMap or ptr difference to +// an uint32_t location without data loss. +template +static inline uint32_t NarrowOffset(T ptr) +{ + uint32_t res = static_cast(ptr); + if (unlikely(ptr < 0)) + abort(); + if (unlikely(static_cast(res) != ptr)) + abort(); + return res; +} + // CacheGenerator::pkgCacheGenerator - Constructor /*{{{*/ // --------------------------------------------------------------------- /* We set the dirty flag and make sure that is written to the disk */ @@ -210,7 +223,7 @@ map_stringitem_t pkgCacheGenerator::WriteStringInMap(const char *String, const unsigned long &Len) { size_t oldSize = Map.Size(); void const * const oldMap = Map.Data(); - map_stringitem_t const index{Map.WriteString(String, Len)}; + map_stringitem_t const index{NarrowOffset(Map.WriteString(String, Len))}; if (index != 0) ReMap(oldMap, Map.Data(), oldSize); return index; @@ -220,7 +233,7 @@ map_stringitem_t pkgCacheGenerator::WriteStringInMap(const char *String, map_stringitem_t pkgCacheGenerator::WriteStringInMap(const char *String) { size_t oldSize = Map.Size(); void const * const oldMap = Map.Data(); - map_stringitem_t const index{Map.WriteString(String)}; + map_stringitem_t const index{NarrowOffset(Map.WriteString(String))}; if (index != 0) ReMap(oldMap, Map.Data(), oldSize); return index; @@ -827,7 +840,7 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver, return false; pkgCache::VerFileIterator VF(Cache,Cache.VerFileP + VerFile); - VF->File = map_pointer{CurrentFile - Cache.PkgFileP}; + VF->File = map_pointer{NarrowOffset(CurrentFile - Cache.PkgFileP)}; // Link it to the end of the list map_pointer *Last = &Ver->FileList; @@ -912,7 +925,7 @@ bool pkgCacheGenerator::NewFileDesc(pkgCache::DescIterator &Desc, return false; pkgCache::DescFileIterator DF(Cache,Cache.DescFileP + DescFile); - DF->File = map_pointer{CurrentFile - Cache.PkgFileP}; + DF->File = map_pointer{NarrowOffset(CurrentFile - Cache.PkgFileP)}; // Link it to the end of the list map_pointer *Last = &Desc->FileList; @@ -1267,7 +1280,7 @@ bool pkgCacheGenerator::SelectReleaseFile(const string &File,const string &Site, CurrentRlsFile->Flags = Flags; CurrentRlsFile->ID = Cache.HeaderP->ReleaseFileCount; RlsFileName = File; - Cache.HeaderP->RlsFileList = map_pointer{CurrentRlsFile - Cache.RlsFileP}; + Cache.HeaderP->RlsFileList = map_pointer{NarrowOffset(CurrentRlsFile - Cache.RlsFileP)}; Cache.HeaderP->ReleaseFileCount++; return true; @@ -1316,11 +1329,11 @@ bool pkgCacheGenerator::SelectFile(std::string const &File, CurrentFile->Component = component; CurrentFile->Flags = Flags; if (CurrentRlsFile != nullptr) - CurrentFile->Release = map_pointer{CurrentRlsFile - Cache.RlsFileP}; + CurrentFile->Release = map_pointer{NarrowOffset(CurrentRlsFile - Cache.RlsFileP)}; else CurrentFile->Release = 0; PkgFileName = File; - Cache.HeaderP->FileList = map_pointer{CurrentFile - Cache.PkgFileP}; + Cache.HeaderP->FileList = map_pointer{NarrowOffset(CurrentFile - Cache.PkgFileP)}; Cache.HeaderP->PackageFileCount++; if (Progress != 0) -- cgit v1.2.3-70-g09d2