Re: [devel] [PATCH apt 3/3] Fix UB in pointer arithmetic

[Aleksei Nikiforov <darktemplar@altlinux.org>]

10.12.2019 2:56, Dmitry V. Levin пишет:
> Commit 6d5e6a689d07de8feef2cbecb24bc42d5994861b aka 0.5.15lorg2-alt70~9
> among other changes introduced UB in pointer arithmetic by casting raw
> pointers to specific types.
> 
> Fix this by introducing two helpers for rebasing pointers in a safe way.
> 
> Co-developed-by: Aleksei Nikiforov <darktemplar@altlinux.org>

This line is not true. I didn't participate in creation of this version 
of patch. Please remove it.

> Fixes: 6d5e6a68 ("apt-pkg/pkgcachegen.{cc,h} changes")
> ---
>   apt/apt-pkg/Makefile.am          |  1 +
>   apt/apt-pkg/cacheiterators.h     | 14 ++++++++------
>   apt/apt-pkg/contrib/mmap.cc      |  8 ++++----
>   apt/apt-pkg/pkgcachegen.cc       | 27 +++++++++++----------------
>   apt/apt-pkg/rebase_pointer.h     | 25 +++++++++++++++++++++++++
>   apt/apt-pkg/rpm/rpmlistparser.cc |  3 ++-
>   6 files changed, 51 insertions(+), 27 deletions(-)
>   create mode 100644 apt/apt-pkg/rebase_pointer.h
> 
> diff --git a/apt/apt-pkg/Makefile.am b/apt/apt-pkg/Makefile.am
> index 4c0d234..d038d01 100644
> --- a/apt/apt-pkg/Makefile.am
> +++ b/apt/apt-pkg/Makefile.am
> @@ -94,6 +94,7 @@ libapt_pkg_la_SOURCES = \
>   	pkgsystem.h \
>   	policy.cc \
>   	policy.h \
> +	rebase_pointer.h \
>   	repository.cc \
>   	repository.h \
>   	scopeexit.h \
> diff --git a/apt/apt-pkg/cacheiterators.h b/apt/apt-pkg/cacheiterators.h
> index 9dffeb3..3c60cb8 100644
> --- a/apt/apt-pkg/cacheiterators.h
> +++ b/apt/apt-pkg/cacheiterators.h
> @@ -34,6 +34,8 @@
>   #pragma interface "apt-pkg/cacheiterators.h"
>   #endif
>   
> +#include <apt-pkg/rebase_pointer.h>
> +
>   // Package Iterator
>   class pkgCache::PkgIterator
>   {
> @@ -87,7 +89,7 @@ class pkgCache::PkgIterator
>      {
>         if (Owner == 0 || Pkg == 0)
>            return;
> -      Pkg += static_cast<Package const *>(newMap) - static_cast<Package const *>(oldMap);
> +      RebasePointer(Pkg, oldMap, newMap);
>      }
>   
>      // Constructors
> @@ -149,7 +151,7 @@ class pkgCache::VerIterator
>      {
>         if (Owner == 0 || Ver == 0)
>            return;
> -      Ver += static_cast<Version const *>(newMap) - static_cast<Version const *>(oldMap);
> +      RebasePointer(Ver, oldMap, newMap);
>      }
>   
>      inline VerIterator() : Ver(0), Owner(0) {};
> @@ -222,7 +224,7 @@ class pkgCache::DepIterator
>      {
>         if (Owner == 0 || Dep == 0)
>            return;
> -      Dep += static_cast<Dependency const *>(newMap) - static_cast<Dependency const *>(oldMap);
> +      RebasePointer(Dep, oldMap, newMap);
>      }
>   
>      inline DepIterator(pkgCache &Owner,Dependency *Trg,Version * = 0) :
> @@ -281,7 +283,7 @@ class pkgCache::PrvIterator
>      {
>         if (Owner == 0 || Prv == 0)
>            return;
> -      Prv += static_cast<Provides const *>(newMap) - static_cast<Provides const *>(oldMap);
> +      RebasePointer(Prv, oldMap, newMap);
>      }
>   
>      inline PrvIterator() : Prv(0), Type(PrvVer), Owner(0)  {};
> @@ -344,7 +346,7 @@ class pkgCache::PkgFileIterator
>      {
>         if (Owner == 0 || File == 0)
>            return;
> -      File += static_cast<PackageFile const *>(newMap) - static_cast<PackageFile const *>(oldMap);
> +      RebasePointer(File, oldMap, newMap);
>      }
>   
>      // Constructors
> @@ -385,7 +387,7 @@ class pkgCache::VerFileIterator
>      {
>         if (Owner == 0 || FileP == 0)
>            return;
> -      FileP += static_cast<VerFile const *>(newMap) - static_cast<VerFile const *>(oldMap);
> +      RebasePointer(FileP, oldMap, newMap);
>      }
>   
>      inline VerFileIterator() : Owner(0), FileP(0) {};
> diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
> index 2064fc4..779d7a6 100644
> --- a/apt/apt-pkg/contrib/mmap.cc
> +++ b/apt/apt-pkg/contrib/mmap.cc
> @@ -30,6 +30,7 @@
>   #include <apt-pkg/configuration.h>
>   #include <apt-pkg/mmap.h>
>   #include <apt-pkg/error.h>
> +#include <apt-pkg/rebase_pointer.h>
>   
>   #include <apti18n.h>
>   
> @@ -285,13 +286,12 @@ std::experimental::optional<map_ptrloc> DynamicMMap::Allocate(unsigned long Item
>         I->Count = size/ItemSize;
>         Pool* oldPools = Pools;
>         auto idxResult = RawAllocate(I->Count*ItemSize,ItemSize);
> -      if (Pools != oldPools)
> -         I += Pools - oldPools;
>   
>         // Does the allocation failed ?
>         if (!idxResult)
>            return idxResult;
>   
> +      RebasePointer(I, oldPools, Pools);
>         Result = *idxResult;
>         I->Start = Result;
>      }
> @@ -356,7 +356,7 @@ bool DynamicMMap::Grow(unsigned long long size)
>         Fd->Write(&C,sizeof(C));
>      }
>   
> -   unsigned long const poolOffset = Pools - ((Pool*) Base);
> +   const void *old_base = Base;
>   
>      if (Fd != 0)
>      {
> @@ -393,7 +393,7 @@ bool DynamicMMap::Grow(unsigned long long size)
>         memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
>      }
>   
> -   Pools = (Pool*) Base + poolOffset;
> +   RebasePointer(Pools, old_base, Base);
>      WorkSpace = newSize;
>   
>      return true;
> diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
> index 56716b5..7a5a20c 100644
> --- a/apt/apt-pkg/pkgcachegen.cc
> +++ b/apt/apt-pkg/pkgcachegen.cc
> @@ -26,6 +26,7 @@
>   #include <apt-pkg/strutl.h>
>   #include <apt-pkg/sptr.h>
>   #include <apt-pkg/pkgsystem.h>
> +#include <apt-pkg/rebase_pointer.h>
>   
>   #include <apti18n.h>
>   
> @@ -116,11 +117,11 @@ void pkgCacheGenerator::ReMap(void const * const oldMap, void const * const newM
>   
>      Cache.ReMap(false);
>   
> -   CurrentFile += (pkgCache::PackageFile*) newMap - (pkgCache::PackageFile*) oldMap;
> +   RebasePointer(CurrentFile, oldMap, newMap);
>   
>      for (size_t i = 0; i < _count(UniqHash); ++i)
>         if (UniqHash[i] != 0)
> -         UniqHash[i] += (pkgCache::StringItem*) newMap - (pkgCache::StringItem*) oldMap;
> +         RebasePointer(UniqHash[i], oldMap, newMap);
>   
>      for (auto i = Dynamic<pkgCache::PkgIterator>::toReMap.begin();
>           i != Dynamic<pkgCache::PkgIterator>::toReMap.end(); ++i)
> @@ -269,11 +270,8 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>   	 continue;
>         }
>   
> -      if (oldMap != Map.Data())
> -      {
> -         Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> -         oldMap = Map.Data();
> -      }
> +      RebasePointer(Last, oldMap, Map.Data());
> +      oldMap = Map.Data();
>   
>         // Skip to the end of the same version set.
>         if (Res == 0)
> @@ -296,8 +294,7 @@ bool pkgCacheGenerator::MergeList(ListParser &List,
>            return _error->Error(_("Error occurred while processing %s (NewVersion%d)"),
>                                 PackageName.c_str(), 1);
>   
> -      if (oldMap != Map.Data())
> -         Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> +      RebasePointer(Last, oldMap, Map.Data());
>         *Last = *verindex;
>   
>         Ver->ParentPkg = Pkg.Index();
> @@ -604,8 +601,9 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
>         for (pkgCache::DepIterator D = Ver.DependsList(); D.end() == false; D++)
>   	 OldDepLast = &D->NextDepends;
>         OldDepVer = Ver;
> -   } else if (oldMap != Owner->Map.Data())
> -      OldDepLast += (map_ptrloc*) Owner->Map.Data() - (map_ptrloc*) oldMap;
> +   } else {
> +      RebasePointer(OldDepLast, oldMap, Owner->Map.Data());
> +   }
>   
>      // Is it a file dependency?
>      if (PackageName[0] == '/')
> @@ -745,11 +743,8 @@ std::experimental::optional<map_ptrloc> pkgCacheGenerator::WriteUniqString(const
>      if ((!Item) || (!idxString))
>         return std::experimental::optional<map_ptrloc>();
>   
> -   if (oldMap != Map.Data())
> -   {
> -      Last += (map_ptrloc*) Map.Data() - (map_ptrloc*) oldMap;
> -      I += (pkgCache::StringItem*) Map.Data() - (pkgCache::StringItem*) oldMap;
> -   }
> +   RebasePointer(Last, oldMap, Map.Data());
> +   RebasePointer(I, oldMap, Map.Data());
>   
>      *Last = *Item;
>   
> diff --git a/apt/apt-pkg/rebase_pointer.h b/apt/apt-pkg/rebase_pointer.h
> new file mode 100644
> index 0000000..2bbabea
> --- /dev/null
> +++ b/apt/apt-pkg/rebase_pointer.h
> @@ -0,0 +1,25 @@
> +#ifndef PKGLIB_REBASE_POINTER_H
> +#define PKGLIB_REBASE_POINTER_H
> +
> +template <class T>
> +static inline T*
> +GetRebasedPointer(T*, const void *, const void *)
> +__attribute__((__warn_unused_result__));
> +
> +template <class T>
> +static inline T*
> +GetRebasedPointer(T* ptr, const void *old_base, const void *new_base)
> +{
> +	// uintptr_t is a type with well-defined integer overflow semantics
> +	uintptr_t diff = (uintptr_t) new_base - (uintptr_t) old_base;
> +	return (T*) ((uintptr_t) ptr + diff);
> +}
> +
> +template <class T>
> +static inline void
> +RebasePointer(T* &ptr, const void *old_base, const void *new_base)
> +{
> +	ptr = GetRebasedPointer(ptr, old_base, new_base);
> +}
> +
> +#endif
> diff --git a/apt/apt-pkg/rpm/rpmlistparser.cc b/apt/apt-pkg/rpm/rpmlistparser.cc
> index 9b2e9ad..4aeb937 100644
> --- a/apt/apt-pkg/rpm/rpmlistparser.cc
> +++ b/apt/apt-pkg/rpm/rpmlistparser.cc
> @@ -25,6 +25,7 @@
>   #include <apt-pkg/strutl.h>
>   #include <apt-pkg/crc-16.h>
>   #include <apt-pkg/tagfile.h>
> +#include <apt-pkg/rebase_pointer.h>
>   
>   #include <apti18n.h>
>   
> @@ -56,7 +57,7 @@ rpmListParser::rpmListParser(RPMHandler *Handler)
>   
>            for (auto iter: *SeenPackages)
>            {
> -            tmp.insert(iter + (static_cast<const char *>(newMap) - static_cast<const char *>(oldMap)));
> +            tmp.insert(GetRebasedPointer(iter, oldMap, newMap));
>            }
>   
>            SeenPackages->swap(tmp);
>