Re: [devel] [SCM] packages/apt: tags/0.5.15lorg2-alt74

[Aleksei Nikiforov <darktemplar@altlinux.org>]

22.10.2019 18:32, Dmitry V. Levin пишет:
> On Tue, Oct 22, 2019 at 03:28:40PM +0300, Anton Farygin wrote:
>> On 22.10.2019 11:32, Dmitry V. Levin wrote:
>>> On Tue, Oct 22, 2019 at 03:01:16AM +0300, Michael Shigorin wrote:
>>>> On Tue, Oct 22, 2019 at 02:42:06AM +0300, Dmitry V. Levin wrote:
>>>>> On Tue, Sep 17, 2019 at 09:05:15AM +0000, Aleksei Nikiforov wrote:
>>>>>> Update of /people/darktemplar/packages/apt.git
>>>>>>       0.5.15lorg2-alt74
>>>>>>       - Added debugging output for allocation functions.
>>>>>>       - Fixed dynamic memory allocation pointer arithmetics issue.
>>>>> Жаль, что вы не дождались первого апреля, но всё равно спасибо,
>>>>> шутка получилась отличная.
>>>> В смысле про fixed?
>>> Именно.
>>>
>>> Этот патч увеличивает расход оперативной памяти apt'ом в среднем в полтора
>>> раза, но не исправляет UB с арифметикой указателей, поскольку MMap::Base
>>> как был невыровненным, так и остался невыровненным.  В отличие от Debian,
>>> между прочим, где по умолчанию MMap::Base выровнен на начало страницы.
>>>

Спасибо, тоже поржал над этим анализом кода.

Да, патч увеличивает расход памяти. В полтора раза звучит страшно, и не 
соответствует действительности. Прошу UB продемонстрировать. Все 
практические тесты демонстрируют исправление данной проблемы с 
указателями. Код из Debian смотрел, возможно, я что-то пропустил, но где 
там разница между их реализацией MMap::Base и моей, которая исправляла 
бы данную проблему? Было бы замечательно, если бы это исправление из 
Debian было продемонстрировано на e2k с apt из ALT.

>> Я не вижу разницы в адресе MMap::Base на Debian и у нас - в обоих
>> случаях он выровнен на начало страницы.
> 
> У Алексея:
> 
>        void *tmp_base = realloc(Base, newSize);
> 
>        if (debug_grow)
>           _error->Warning(_("DynamicMMap::Grow: realloc from %llu to %llu, result: %s"), WorkSpace, newSize, (tmp_base == n
> 
>        if (tmp_base == NULL)
>           return false;
> 
>        Base = tmp_base;
> 
> В Debian по умолчанию:
> 
>     #ifdef MREMAP_MAYMOVE
> 
>                  if ((Flags & Moveable) == Moveable)
>                          Base = mremap(Base, WorkSpace, newSize, MREMAP_MAYMOVE);
>                  else
>     #endif
>                          Base = mremap(Base, WorkSpace, newSize, 0);
> 
>                  if(Base == MAP_FAILED)
>                          return false;
> 
> Всё ещё не видно разницы?
> 

Предлагаю посмотреть внимательнее. Вот мой код:

    if (Fd != 0)
    {
       void *tmp_base = MAP_FAILED;
#ifdef MREMAP_MAYMOVE
       if ((this->Flags & Moveable) == Moveable)
          tmp_base = mremap(Base, WorkSpace, newSize, MREMAP_MAYMOVE);
       else
#endif
          tmp_base = mremap(Base, WorkSpace, newSize, 0);

       if (debug_grow)
          _error->Warning(_("DynamicMMap::Grow: mremap from %llu to 
%llu, result: %s"), WorkSpace, newSize, (tmp_base == MAP_FAILED) ? 
_("Fail") : _("Success"));

       if (tmp_base == MAP_FAILED)
          return false;

       Base = tmp_base;
    } else {
       if ((this->Flags & Moveable) != Moveable)
          return false;

       void *tmp_base = realloc(Base, newSize);

       if (debug_grow)
          _error->Warning(_("DynamicMMap::Grow: realloc from %llu to 
%llu, result: %s"), WorkSpace, newSize, (tmp_base == nullptr) ? 
_("Fail") : _("Success"));

       if (tmp_base == NULL)
          return false;

       Base = tmp_base;

       /* Set new memory to 0 */
       memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
    }


А вот код из Debian:

	if ((Flags & Fallback) != Fallback) {
#if defined(_POSIX_MAPPED_FILES) && defined(__linux__)
    #ifdef MREMAP_MAYMOVE

		if ((Flags & Moveable) == Moveable)
			Base = mremap(Base, WorkSpace, newSize, MREMAP_MAYMOVE);
		else
    #endif
			Base = mremap(Base, WorkSpace, newSize, 0);

		if(Base == MAP_FAILED)
			return false;
#else
		return false;
#endif
	} else {
		if ((Flags & Moveable) != Moveable)
			return false;

		Base = realloc(Base, newSize);
		if (Base == NULL)
			return false;
		else
			/* Set new memory to 0 */
			memset((char*)Base + WorkSpace, 0, newSize - WorkSpace);
	}