ALT Linux Bugzilla – #41309

(Ответ для annschafer на комментарий #0)
> Версия - 0.4.5-alt13
> 
> При попытке открытия "Пакет" - "Changelog", предварительно выбрав
> виртуальный пакет, завершение с ошибкой сегментирования.

Причём если использовать привязанную к пункту меню клавишу (C), то ничего не происходит. (А у нормальных пакетов также успешно показывается changelog.)

Я посмотрел с помощью gdb место падения, которое оказалось в libapt в pkgRecords::Lookup(), и сделал вывод, что причиной этого скорее всего использование где-то раньше по ходу выполнения aptitude объекта VerIterator без проверки, что он указывает на конец (пустой список). К моменту pkgRecords::Lookup() работа уже идёт с мусором из памяти, в которой хранятся совсем другие данные. Моё внимание тут больше, чем само поведение aptitude, привлекла возможность вот так перескочить на недействительные данные. Записал комментарий, что можно было бы код libapt сделать более устойчивым к такому:

investigate a crash in pkgRecords::Lookup() (called from aptitude to display "the changelog" of a virtual pkg)

First, I wrote down some assumptions (as assertions): that the used
iterators should not point to the end etc.

My guess is that it's this assumption that is broken when aptitude
crashes on an attempt to display "the changelog" of a virtual package
through the menu (not through the press of the hotkey, "C").

And there can be plenty of such places in code, where an iterator is used
without checking whether it points to the end... And some accessors in
the iterator classes do check this, some (like VerFileIterator::File())
don't... what a mess! I mean all kind of accessors; here's an example
where they differ in behavior:

   // Accessors
   inline Version *operator ->() {return Ver;}
   inline Version const *operator ->() const {return Ver;}
   inline Version &operator *() {return *Ver;}
   inline Version const &operator *() const {return *Ver;}
   inline operator Version *() {return Ver == Owner->VerP?0:Ver;}
   inline operator Version const *() const {return Ver == Owner->VerP?0:Ver;}


Cf. how "apt-cache show" handles the case of a virtual package specially:

      // CNC:2004-07-09
      // If it's a virtual package, require user to select similarly to apt-get
      if (Pkg.VersionList().end() == true and Pkg->ProvidesList != 0)
      {
         ioprintf(cout, _("Package %s is a virtual package provided by:\n"),
                  Pkg.Name());
         ...
         return false;
      }

      // Find the proper version to use.

Conclusion: neither of the assertions fired in on the crash.

Ultimately, gdb with -O0 pointed at the place where a PackageFile
struct is read:

   const pkgCache::PkgFileIterator PkgFile = Ver.File();
   pkgCache::PackageFile const PackageFile = *PkgFile;

So, there is just some arbitary wrong data in these iterators, and the
reason is another iterator (VerIterator for a virtual package) that
was used before (in aptitude), and which actually is an empty list
(points to the end), but this is not checked there. And since a
pointer to the end looks like a normal pointer to Owner->VerP (memory
offset 0), and most methods of the iterators don't check it, it just
continued working with arbitrary wrong data from that memory region.

Improvements:

1. rewrite iterators (with a base template), so that whenever the
underlying pointer is set, it is checked for being the end, and if so,
immediately converted into nullptr. Then the crashes would happen much
earlier, at the actual place of incorrect use of an iterator. After this
change, no overhead for checking the pointer will be needed in the accessors.

2. (related) parameterize and encapsulate map_ptrloc (as scoped enum),
with UnpackPtr() and PackPtr() (the latter calculating the offset).
It's related to the previous change, because when we start returning
nullptr, some places where the offset is calculated may get incorrect:
it should become offset 0, not (0 - Owner->VerP) etc.; and if assigning
such a value can only be done via PackPtr(), we can be sure that it takes
care of the correct calculation.

3. Actually, there was no need to introduce std::optional in the past
when we allocate and get map_ptrloc's (the offsets), because offset 0
can be an unambiguous sign of failure; offset zero is not allowed for real data
(except for HeederP perhaps) and is a sign of pointing at the end.

4. Perhaps: avoid conversion operators from iterators to the
underlying pointer (and get rid of them), because they make the code
less clear and also make it less clear where the underlying address is
actually used and can lead to incorrect interpretation (end vs
non-end: is the end nullptr or Owner-VerP), particularly, when
possibly calculating the offset based on such value.

Результат проверки актуальности

Состояние:
aptitude 0.4.5-alt16. Changelog виртуального пакета всё ещё Lookup по пустому VerIterator.

Рекомендация:
оставить открытым.