Re: [devel] re APT patch with impossible error on "Can't allocate an item of size zero"

[Ivan Zakharyaschev <imz@altlinux.org>]

[-- Attachment #1: Type: text/plain, Size: 9894 bytes --]

Hello!

On Thu, 13 Feb 2020, Ivan Zakharyaschev wrote:

> > > есть такой hunk касательно DynamicMMap::RawAllocate():
> > > 
> > > diff --git a/apt/apt-pkg/contrib/mmap.cc b/apt/apt-pkg/contrib/mmap.cc
> > > index d7a5c3a68..36dc11524 100644
> > > --- a/apt/apt-pkg/contrib/mmap.cc
> > > +++ b/apt/apt-pkg/contrib/mmap.cc
> > > @@ -226,6 +245,12 @@ unsigned long DynamicMMap::RawAllocate(unsigned long
> > > Size,unsigned long Aln)
> > >      size in the file. */
> > >   unsigned long DynamicMMap::Allocate(unsigned long ItemSize)
> > >   {
> > > +   if (ItemSize == 0)
> > > +   {
> > > +      _error->Error("Can't allocate an item of size zero");
> > > +      return 0;
> > > +   }
> > > +
> > >      // Look for a matching pool entry
> > >      Pool *I;
> > >      Pool *Empty = 0;

> > > Во-вторых, я подумал, что это такой класс ошибок, который не возникает
> > > в run-time из-за плохих данных или отказа ОС (например, выделить
> > > память или прочитать файл) -- т.е. не что-то заранее непредсказуемое
> > > для программиста и поэтому требующее специальной обработки в run-time.
> > > 
> > > Если програмист написал всё так, как он хотел, то он не передаст
> > > неправильный аргумент (0) в функцию. (Да и понятно, что в реальном
> > > коде APT это не ожидается: там обычно этот аргумент заполняется
> > > константой sizeof(...).)
> > > 
> > 
> > Ты упускаешь большой момент. mmap.h - публичный заголовок библиотеки libapt.
> > Помимо самого apt, им пользуется ещё и немало клиентов. Даже если в самом apt
> > не найдётся способа неправильно вызвать данную функцию, это ничего не значит.
> > Не проверять входные данные - плохая идея.
> 
> Всё равно это ответственность программиста: правильно вызвать.
> 
> Я выше написал, в каких случаях я счиатю нужным проверять данные.
> 
> > > Я бы предложил такие требования (имеющие характер спецификации того,
> > > что программист задумал реализовать) оформлять просто с помощью
> > > assert.
> > > 
> > 
> > assert в не-debug сборке отсутствует. Потому чуть более чем полностью
> > бесполезен не во время разработки приложения.
> 
> Я так и задумывал.
> 
> 
> > Это не средство проверки входных
> > данных. Поэтому я против.
> > 
> > > Так будет легче понимать код: сразу ясно, что выполнение этого условия
> > > (важного для корректности кода в теле функции) просто гарантируется
> > > тем, как весь наш код написан и вообще-то может быть обосновано ещё до
> > > run-time (просто глядя на исходный код, на том же этапе работы, что и
> > > запускаем компилятор).

Появилась идея, что есть способ здесь не просто ответственность возложить 
на программиста на словах (вызывать Allocate() с ненулевым аргументом), но 
и формально предложить такой способ программирования, где не будет 
возможности делать не так, как задумано.

Да и попроще вызовы получаются.

Ветка alloc-templates в 
http://git.altlinux.org/people/imz/packages/apt.git

(Развивая это, можно RawAllocate() превратить в какой-нибудь
template<typename T> RawAllocateArray<T>(unsigned long ItemCount),
там, правда, Pools нетривиально надо будет переделать.

Пришлось, правда, перенести определение из .cc в .h и патчи дальнейшие 
неудобно прикладывать; но можно их ifdef-ами обложить в .cc и заинклудить 
в .h.)

>From 89499e5810575336e6a3b02d71d6c323321a0eca Mon Sep 17 00:00:00 2001
From: Ivan Zakharyaschev <imz@altlinux.org>
Date: Fri, 14 Feb 2020 04:04:42 +0300
Subject: [PATCH] don't pass compile-time constants as an argument to
 DynamicMMap::Allocate()

The argument was used for the size of the item we want to allocate.

Instead of an argument, use a parameter for the template DynamicMMap::Allocate()
which denotes the type of the item.

Now, the truth of our assertion that the size is not zero is evident
(and checked by the compiler).

Note that this change may also potentially lead to more optimized code
due to compile-time specialization for each particular size.
---
 apt/apt-pkg/contrib/mmap.h | 12 ++++++------
 apt/apt-pkg/pkgcachegen.cc | 19 ++++++++++---------
 apt/apt-pkg/pkgcachegen.h  |  3 ++-
 3 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/apt/apt-pkg/contrib/mmap.h b/apt/apt-pkg/contrib/mmap.h
index 5b14ac9d9..7ec0fb8b1 100644
--- a/apt/apt-pkg/contrib/mmap.h
+++ b/apt/apt-pkg/contrib/mmap.h
@@ -94,7 +94,8 @@ class DynamicMMap : public MMap
 
    // Allocation
    std::experimental::optional<unsigned long> RawAllocate(unsigned long Size,unsigned long Aln = 0);
-   std::experimental::optional<unsigned long> Allocate(unsigned long ItemSize);
+   template<typename T>
+   std::experimental::optional<unsigned long> Allocate();
    std::experimental::optional<unsigned long> WriteString(const char *String,unsigned long Len = std::numeric_limits<unsigned long>::max());
    inline std::experimental::optional<unsigned long> WriteString(const string &S) {return WriteString(S.c_str(),S.length());};
    void UsePools(Pool &P,unsigned int Count) {Pools = &P; PoolCount = Count;};
@@ -107,17 +108,16 @@ class DynamicMMap : public MMap
 /* Templates */
 
 #include <apt-pkg/error.h>
-#include <cassert>
 
 // DynamicMMap::Allocate - Pooled aligned allocation			/*{{{*/
 // ---------------------------------------------------------------------
 /* This allocates an Item of size ItemSize so that it is aligned to its
    size in the file. */
-std::experimental::optional<unsigned long> DynamicMMap::Allocate(unsigned long ItemSize)
+template<typename T>
+std::experimental::optional<unsigned long> DynamicMMap::Allocate()
 {
-   assert(ItemSize != 0); /* Actually, we are always called with sizeof(...)
-                             compile-time non-zero constant as the argument.
-                          */
+   constexpr unsigned long ItemSize = sizeof(T);
+   static_assert(ItemSize != 0);
 
    // Look for a matching pool entry
    Pool *I;
diff --git a/apt/apt-pkg/pkgcachegen.cc b/apt/apt-pkg/pkgcachegen.cc
index 4bbab07bf..2a75f31e6 100644
--- a/apt/apt-pkg/pkgcachegen.cc
+++ b/apt/apt-pkg/pkgcachegen.cc
@@ -112,8 +112,9 @@ std::experimental::optional<unsigned long> pkgCacheGenerator::WriteStringInMap(c
    return Map.WriteString(String);
 }
 									/*}}}*/
-std::experimental::optional<unsigned long> pkgCacheGenerator::AllocateInMap(unsigned long size) {/*{{{*/
-   return Map.Allocate(size);
+template<typename T>
+std::experimental::optional<unsigned long> pkgCacheGenerator::AllocateInMap() {/*{{{*/
+   return Map.Allocate<T>();
 }
 									/*}}}*/
 // CacheGenerator::MergeList - Merge the package list			/*{{{*/
@@ -376,7 +377,7 @@ bool pkgCacheGenerator::NewPackage(pkgCache::PkgIterator &Pkg, const string &Nam
 #endif
        
    // Get a structure
-   const auto Package = AllocateInMap(sizeof(pkgCache::Package));
+   const auto Package = AllocateInMap<pkgCache::Package>();
    const auto idxName = WriteStringInMap(Name);
    if ((!Package) || (!idxName))
       return false;
@@ -429,7 +430,7 @@ bool pkgCacheGenerator::NewFileVer(pkgCache::VerIterator &Ver,
       return true;
    
    // Get a structure
-   const auto VerFile = AllocateInMap(sizeof(pkgCache::VerFile));
+   const auto VerFile = AllocateInMap<pkgCache::VerFile>();
    if (!VerFile)
       return false;
    
@@ -460,7 +461,7 @@ std::experimental::optional<unsigned long> pkgCacheGenerator::NewVersion(pkgCach
 					    unsigned long Next)
 {
    // Get a structure
-   const auto Version = AllocateInMap(sizeof(pkgCache::Version));
+   const auto Version = AllocateInMap<pkgCache::Version>();
    const auto idxVerStr = WriteStringInMap(VerStr);
    if ((!Version) || (!idxVerStr))
       return std::experimental::nullopt;
@@ -487,7 +488,7 @@ bool pkgCacheGenerator::ListParser::NewDepends(pkgCache::VerIterator &Ver,
    pkgCache &Cache = Owner->Cache;
    
    // Get a structure
-   const auto Dependency = Owner->AllocateInMap(sizeof(pkgCache::Dependency));
+   const auto Dependency = Owner->AllocateInMap<pkgCache::Dependency>();
    if (!Dependency)
       return false;
    
@@ -574,7 +575,7 @@ bool pkgCacheGenerator::ListParser::NewProvides(pkgCache::VerIterator &Ver,
       }
    }
    // Get a structure
-   const auto Provides = Owner->AllocateInMap(sizeof(pkgCache::Provides));
+   const auto Provides = Owner->AllocateInMap<pkgCache::Provides>();
    if (!Provides)
       return false;
    /* Note:
@@ -618,7 +619,7 @@ bool pkgCacheGenerator::SelectFile(const string &File, const string &Site,
 				   unsigned long Flags)
 {
    // Get some space for the structure
-   const auto idxFile = AllocateInMap(sizeof(*CurrentFile));
+   const auto idxFile = AllocateInMap<decltype(*CurrentFile)>();
    const auto idxFileName = WriteStringInMap(File);
    const auto idxSite = WriteUniqString(Site);
    const auto idxIndexType = WriteUniqString(Index.GetType()->Label);
@@ -676,7 +677,7 @@ std::experimental::optional<unsigned long> pkgCacheGenerator::WriteUniqString(co
    }
    
    // Get a structure
-   const auto Item = AllocateInMap(sizeof(pkgCache::StringItem));
+   const auto Item = AllocateInMap<pkgCache::StringItem>();
    const auto idxString = WriteStringInMap(S, Size);
    if ((!Item) || (!idxString))
       return std::experimental::nullopt;
diff --git a/apt/apt-pkg/pkgcachegen.h b/apt/apt-pkg/pkgcachegen.h
index b789177ef..73252802b 100644
--- a/apt/apt-pkg/pkgcachegen.h
+++ b/apt/apt-pkg/pkgcachegen.h
@@ -39,7 +39,8 @@ class pkgCacheGenerator
    std::experimental::optional<unsigned long> WriteStringInMap(const std::string &String) { return WriteStringInMap(String.c_str(), String.length()); };
    std::experimental::optional<unsigned long> WriteStringInMap(const char *String);
    std::experimental::optional<unsigned long> WriteStringInMap(const char *String, unsigned long Len);
-   std::experimental::optional<unsigned long> AllocateInMap(unsigned long size);
+   template<typename T>
+   std::experimental::optional<unsigned long> AllocateInMap();
 
    public:
    
-- 
2.21.0