Арифметика указателей UB в throw_realloc_debug()

Nil A написал(а) к Vitaliy Aksyonov в Oct 23 21:35:16 по местному времени:

Нello, Vitaliy!

Во-первых, зачем по-умолчанию включены отладки GTНROWLOG и GTНROWDEBUG? Я иногда видел какие-то трейсы про память пишутся в golded.log, но реально, хоть что-то когда-нибудь присылал сюда в эху, или куда-то, какие-то отладочные трейсы памяти?

Отладка GTНROW_DEBUG в голдеде сделана таким образом, что они аллоцируют памяти больше на структурку Throw, и прячут её в начале. Перед адресом и после обкладывают магическими цифирками BEFOREVAL и AFTERVAL, чтобы потом на недоезды и заезды проверить. Также они двухсвязный список хранят, чтобы потом потерянную память напечатать. А потом пришёл Гугл и запили ASAN билды, и такое делать в ручную уже стало не нужно, так что выключить по-умолчанию я бы рекомендовал.


Решилось мне прогнать UBSAN билд, и прямо на запуске, где происходит сканирование всех областей (баз эх), случается UB

/home/fido/src/golded-plus/goldlib/gall/gmemdbg.cpp:130:53: runtime error: pointer index expression with base 0x000000000000 overflowed to 0xffffffffffffffd4
#0 0xae8fbc in throw_ptrtodl(void const*) /home/fido/src/golded-plus/goldlib/gall/gmemdbg.cpp:130
#1 0xae6a1c in throwreallocdebug(void, unsigned long, char const, int) /home/fido/src/golded-plus/goldlib/gall/gmemdbg.cpp:389
#2 0x94880f in SquishArea::refresh() /home/fido/src/golded-plus/goldlib/gmb3/gmosqsh2.cpp:71
#3 0x94b7f1 in SquishArea::raw_scan(int, int) /home/fido/src/golded-plus/goldlib/gmb3/gmosqsh2.cpp:153
#4 0x94fd9b in SquishArea::scan_area() /home/fido/src/golded-plus/goldlib/gmb3/gmosqsh2.cpp:333
#5 0x7ebee5 in Area::ScanArea() /home/fido/src/golded-plus/golded3/gescan.cpp:71
#6 0x7efc77 in AreaList::AreaScan(int, unsigned int, int, int&, int&, char const*) /home/fido/src/golded-plus/golded3/gescan.cpp:281
#7 0x7b236f in Reader() /home/fido/src/golded-plus/golded3/geread.cpp:171
#8 0x6c63bb in main /home/fido/src/golded-plus/golded3/gemain.cpp:54
#9 0x7f7f53f1ef44 in _libc_start_main (/lib/x8664-linux-gnu/libc.so.6+0x21f44)
#10 0x407c98 (/home/fido/src/golded-plus/build_asan/golded3/golded+0x407c98)

Тут хотят вычитать целиком .SQI индекс файл в память. Зовут функцию throw_realloc(), где оригинальный указатель в NULL. Так то норм, звать realloc с nullptr, тогда он просто в malloc() превращается.

Вобщем-то бага то и нет, но всё равно, я бы почиНИЛ throwrealloc_debug(), пододвинул throwptrtodl() туда, где он реально используется. Т.е. вот такой фикс бы влил, но.. у меня нет на гитхабе аккаунта.

--- a/goldlib/gall/gmemdbg.cpp
+++ b/goldlib/gall/gmemdbg.cpp
@@ -386,7 +386,6 @@ void throwrealloc_debug(void* __oldptr, size_t __size, const char _file, int
{

void* _ptr;
- Throw* dl = throwptrtodl(_oldptr);

if(size == 0)
{
@@ -399,6 +398,7 @@ void throwrealloc_debug(void* __oldptr, size_t __size, const char _file, int
}
else
{
+ Throw* dl = throwptrtodl(_oldptr);
ptr = throw_malloc_debug(__size,__file,_line);
if(dl->nbytes < size)
size = dl->nbytes;


Best Regards, Nil
--- GoldED+/LNX 1.1.5

Vitaliy Aksyonov написал(а) к Nil A в Oct 23 22:59:40 по местному времени:

Привет, Nil!

13 Oct 23 21:35, ты писал(а) мне:

NA> Во-первых, зачем по-умолчанию включены отладки GTНROW_LOG и
NA> GTНROW_DEBUG? Я иногда видел какие-то трейсы про память пишутся в
NA> golded.log, но реально, хоть что-то когда-нибудь присылал сюда в эху,
NA> или куда-то, какие-то отладочные трейсы памяти?

Подозреваю, это сделали, чтобы было проще собирать отчеты об ошибках. Коряво, конечно. Для релизной сборки это все должно быть отключено. Оно и на производительность влияет не самым благоприятным образом.

Я посмотрю, как это удобно сделать отключаемым для релизной сборки. Основная проблема, что там целая куча систем сборки, начиная от cmake, заканчивая пачкой makefiles для разных систем.

NA> Отладка GTНROW_DEBUG в голдеде сделана таким образом, что они
NA> аллоцируют памяти больше на структурку Throw, и прячут её в начале.
NA> Перед адресом и после обкладывают магическими цифирками BEFOREVAL и
NA> AFTERVAL, чтобы потом на недоезды и заезды проверить. Также они
NA> двухсвязный список хранят, чтобы потом потерянную память напечатать. А
NA> потом пришёл Гугл и запили ASAN билды, и такое делать в ручную уже
NA> стало не нужно, так что выключить по-умолчанию я бы рекомендовал.

Мало того, realloc совсем не обязательно должен выделять память в новом месте. В случае с уменьшением куска, это вполне может быть тот же кусок. Другое дело, что в таком виде это вряд ли используется.

NA> Решилось мне прогнать UBSAN билд, и прямо на запуске, где происходит
NA> сканирование всех областей (баз эх), случается UB

[...skipped...]

NA> Вобщем-то бага то и нет, но всё равно, я бы почиНИЛ
NA> throwrealloc_debug(), пододвинул throwptrtodl() туда, где он реально
NA> используется. Т.е. вот такой фикс бы влил, но.. у меня нет на гитхабе
NA> аккаунта.

Создал Pull Request с твоей правкой. Ожидайте на линии, Ваш звонок очень важен для нас. :))

Best regards,
Vitaliy Aksyonov.

... Были бы мозги, было б сотрясение.
--- GoldED+/LNX 1.1.5-b20231007

Semen Panevin написал(а) к Vitaliy Aksyonov в Oct 23 08:27:46 по местному времени:

Доброго здоровьица тебе, Vitaliy!

Monday October 16 2023 22:59, Vitaliy Aksyonov писал Nil A:

NA>> Во-первых, зачем по-умолчанию включены отладки GTНROW_LOG и
NA>> GTНROW_DEBUG? Я иногда видел какие-то трейсы про память пишутся в
NA>> golded.log, но реально, хоть что-то когда-нибудь присылал сюда в
NA>> эху, или куда-то, какие-то отладочные трейсы памяти?

VA> Подозреваю, это сделали, чтобы было проще собирать отчеты об ошибках.
VA> Коряво, конечно. Для релизной сборки это все должно быть отключено.
VA> Оно и на производительность влияет не самым благоприятным образом.

VA> Я посмотрю, как это удобно сделать отключаемым для релизной сборки.
VA> Основная проблема, что там целая куча систем сборки, начиная от cmake,
VA> заканчивая пачкой makefiles для разных систем.
Ну там же есть глобальный Makefile в корне. Где всякие PLATFORM=xxx, ICONV=1 и прочие OLDSНIFTFN=1. Напрашивается добавить туда же DEBUG=1 или чё-нить типа того, ИМХО.

С наилучшими пожеланиями, Семён.

... От правды далеко не убежишь (с) Sage
--- GoldED+/LNX 1.1.5-b20231008 (Linux 6.1.53-gentoo-r1 iF6M10)

Vitaliy Aksyonov написал(а) к Semen Panevin в Oct 23 23:47:46 по местному времени:

Привет, Semen!

17 Oct 23 08:27, ты писал(а) мне:

NA>>> Во-первых, зачем по-умолчанию включены отладки GTНROW_LOG и
NA>>> GTНROW_DEBUG? Я иногда видел какие-то трейсы про память пишутся
NA>>> в golded.log, но реально, хоть что-то когда-нибудь присылал сюда
NA>>> в эху, или куда-то, какие-то отладочные трейсы памяти?

VA>> Подозреваю, это сделали, чтобы было проще собирать отчеты об
VA>> ошибках. Коряво, конечно. Для релизной сборки это все должно быть
VA>> отключено. Оно и на производительность влияет не самым
VA>> благоприятным образом.

VA>> Я посмотрю, как это удобно сделать отключаемым для релизной
VA>> сборки. Основная проблема, что там целая куча систем сборки,
VA>> начиная от cmake, заканчивая пачкой makefiles для разных систем.
SP> Ну там же есть глобальный Makefile в корне. Где всякие PLATFORM=xxx,
SP> ICONV=1 и прочие OLDSНIFTFN=1. Напрашивается добавить туда же
SP> DEBUG=1 или чё-нить типа того, ИМХО.

Не все так просто. Там уже есть и DEBUG, и NDEBUG. Надо внимательно смотреть, чтобы не отломать что-то.

Best regards,
Vitaliy Aksyonov.

... Скажи мне с кем ты пьёшь и я скажу кто ты.
--- GoldED+/LNX 1.1.5-b20231007

Nil A написал(а) к Vitaliy Aksyonov в Oct 23 08:53:10 по местному времени:

Нello, Vitaliy!

Monday October 16 2023 22:59, from Vitaliy Aksyonov -> Nil A:

NA>> Во-первых, зачем по-умолчанию включены отладки GTНROW_LOG и
NA>> GTНROW_DEBUG? Я иногда видел какие-то трейсы про память пишутся в
NA>> golded.log, но реально, хоть что-то когда-нибудь присылал сюда в
NA>> эху, или куда-то, какие-то отладочные трейсы памяти?

VA> Подозреваю, это сделали, чтобы было проще собирать отчеты об ошибках.
VA> Коряво, конечно. Для релизной сборки это все должно быть отключено.
VA> Оно и на производительность влияет не самым благоприятным образом.

VA> Мало того, realloc совсем не обязательно должен выделять память в
VA> новом месте. В случае с уменьшением куска, это вполне может быть тот
VA> же кусок. Другое дело, что в таком виде это вряд ли используется.

Этот GTНROW_DEBUG ловит только аллокации в куче и какие-то минимальные заезды, тогда как тулы, в лице ASAN билдов, и valgrind, ловят и заезды на стеке тоже, коих в этот эхотаге $(grep strcpy) и маленькая тележка ;-)

Best Regards, Nil
--- GoldED+/LNX 1.1.5

Vitaliy Aksyonov написал(а) к Nil A в Oct 23 09:26:16 по местному времени:

Привет, Nil!

17 Oct 23 08:53, ты писал(а) мне:

NA>>> Во-первых, зачем по-умолчанию включены отладки GTНROW_LOG и
NA>>> GTНROW_DEBUG? Я иногда видел какие-то трейсы про память пишутся
NA>>> в golded.log, но реально, хоть что-то когда-нибудь присылал сюда
NA>>> в эху, или куда-то, какие-то отладочные трейсы памяти?
VA>> Подозреваю, это сделали, чтобы было проще собирать отчеты об
VA>> ошибках. Коряво, конечно. Для релизной сборки это все должно быть
VA>> отключено. Оно и на производительность влияет не самым
VA>> благоприятным образом.

VA>> Мало того, realloc совсем не обязательно должен выделять память в
VA>> новом месте. В случае с уменьшением куска, это вполне может быть
VA>> тот же кусок. Другое дело, что в таком виде это вряд ли
VA>> используется.

NA> Этот GTНROW_DEBUG ловит только аллокации в куче и какие-то минимальные
NA> заезды, тогда как тулы, в лице ASAN билдов, и valgrind, ловят и заезды
NA> на стеке тоже, коих в этот эхотаге $(grep strcpy) и маленькая тележка
NA> ;-)

Я это прекрасно понимаю. :) Плюс текущего подхода, что не надо распространять сборку с санитайзером и все равно есть шанс поймать подобные ошибки у пользователя. Часто бывает, что ошибка воспроизводится только при крайне специфичных условиях. А вообще, конечно, надо этот легаси выпиливать.

Best regards,
Vitaliy Aksyonov.

... Федерация Исключительного Дружеского Общения.
--- GoldED+/LNX 1.1.5-b20231007