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

[Andrey Savchenko <bircoph@altlinux.org>]

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

On Thu, 24 Oct 2019 16:59:13 +0300 Aleksei Nikiforov wrote:
> 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 продемонстрировать.

UB не обязательно демонстрировать. Если по стандарту языка
происходит 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);
> 	}
> _______________________________________________
> Devel mailing list
> Devel@lists.altlinux.org
> https://lists.altlinux.org/mailman/listinfo/devel

Best regards,
Andrew Savchenko

[-- Attachment #2: Type: application/pgp-signature, Size: 833 bytes --]