Bug 12135

Данный конкретный эффект у меня не воспроизводится. Если возможно, сделайте,
пожалуйста backtrace падения второго запуска scrollkeeper-update, и приаатачьте
к багу.

# scrollkeeper-update
*** buffer overflow detected ***: scrollkeeper-update terminated
======= Backtrace: =========
/lib/libc.so.6(__chk_fail+0x41)[0xa7d421c1]
/lib/libc.so.6[0xa7d42908]
/usr/lib/libscrollkeeper.so.0(create_database_directory+0x219)[0xa7da0799]
scrollkeeper-update[0x8049692]
/lib/libc.so.6(__libc_start_main+0xdc)[0xa7c8c05c]
scrollkeeper-update[0x8048e01]
======= Memory map: ========
08048000-0804b000 r-xp 00000000 03:06 46151757   /usr/bin/scrollkeeper-update
0804b000-0804c000 rw-p 00002000 03:06 46151757   /usr/bin/scrollkeeper-update
0804c000-0806d000 rw-p 0804c000 00:00 0          [heap]
a7b97000-a7ba1000 r-xp 00000000 03:02 17133619   /lib/libgcc_s.so.1
a7ba1000-a7ba2000 rw-p 0000a000 03:02 17133619   /lib/libgcc_s.so.1
a7bcf000-a7c02000 r--p 00000000 03:06 34140550  
/usr/lib/locale/ru_RU.cp1251/LC_CTYPE
a7c02000-a7c03000 rw-p a7c02000 00:00 0
a7c03000-a7c36000 r-xp 00000000 03:06 67237778   /usr/lib/libxslt.so.1.1.20
a7c36000-a7c37000 rw-p 00032000 03:06 67237778   /usr/lib/libxslt.so.1.1.20
a7c37000-a7c5b000 r-xp 00000000 03:02 17075481   /lib/libm-2.5.so
a7c5b000-a7c5d000 rw-p 00023000 03:02 17075481   /lib/libm-2.5.so
a7c5d000-a7c70000 r-xp 00000000 03:02 17030874   /lib/libz.so.1.2.3
a7c70000-a7c71000 rw-p 00012000 03:02 17030874   /lib/libz.so.1.2.3
a7c71000-a7c72000 rw-p a7c71000 00:00 0
a7c72000-a7c74000 r-xp 00000000 03:02 17031229   /lib/libdl-2.5.so
a7c74000-a7c76000 rw-p 00001000 03:02 17031229   /lib/libdl-2.5.so
a7c76000-a7d95000 r-xp 00000000 03:02 17031203   /lib/libc-2.5.so
a7d95000-a7d96000 r--p 0011f000 03:02 17031203   /lib/libc-2.5.so
a7d96000-a7d98000 rw-p 00120000 03:02 17031203   /lib/libc-2.5.so
a7d98000-a7d9b000 rw-p a7d98000 00:00 0
a7d9b000-a7da2000 r-xp 00000000 03:06 67495016   /usr/lib/libscrollkeeper.so.0.0.0
a7da2000-a7da3000 rw-p 00007000 03:06 67495016   /usr/lib/libscrollkeeper.so.0.0.0
a7da3000-a7eb8000 r-xp 00000000 03:06 67207005   /usr/lib/libxml2.so.2.6.28
a7eb8000-a7ebe000 rw-p 00114000 03:06 67207005   /usr/lib/libxml2.so.2.6.28
a7ede000-a7ee5000 r--s 00000000 03:06 54595236   /usr/lib/gconv/gconv-modules.cache
a7ee5000-a7eeb000 r--p 00000000 03:06 34140551  
/usr/lib/locale/ru_RU.cp1251/LC_COLLATE
a7eeb000-a7eec000 rw-p a7eeb000 00:00 0
a7eec000-a7eed000 r-xp a7eec000 00:00 0          [vdso]
a7eed000-a7f04000 r-xp 00000000 03:02 17031192   /lib/ld-2.5.so
a7f04000-a7f06000 rw-p 00017000 03:02 17031192   /lib/ld-2.5.so
afb7c000-afb91000 rw-p afb7c000 00:00 0          [stack]
Aborted
#

Сизиф сегодняшний, из пакетов стоит всё новое.

Created attachment 2040 [details]
Лог scrollkeeper-update сегодня.

Данный лог был создан сегодня. В последний раз был запущен второй проход
scrollkeeper-update.

Кстати, я совсем не понял: а откуда появился каталог scrollkeeper в корне
системы? Он же должен быть в /var/lib? Ни одному пакету он не принадлежит, дата
создания: сегодня, 25 июня. До этого его не было.

Это ошибка упаковки была, в -alt5 исправлено. Backtrace такой же, как от первого
запуска, похоже, create_desktop_database передаёт NULL в readdir, или что-то
вроде того. Под отладкой бы его пройти...

Created attachment 2041 [details]
Трассировка ltrace

Запуск программы под ltrace

Created attachment 2042 [details]
Трассировка strace

Запуск программы под strace.

Так, ковыряние в scrollkeeper привело к тому, что ошибка оказалась в команде
realpath(source_path,aux_path)
в файле libs/database.c в функции create_database_directory, когда в source_path
оказывается символьная ссылка /usr/share/scrollkeeper/Templates/no

create_database_directory находится в libscrollkeeper.so.0

Проблема в том, что PATHLEN в виде 256 не хватает. Заменив его на MAXPATHLEN (и
подключив sys/param.h), ошибка исчезла (кусок кода из libs/database.c):

        else /* link the directory */
        {
            char *target_locale;
//Замена PATHLEN на MAXPATHLEN из sys/param.h
            char aux_path[MAXPATHLEN];

            realpath(source_path, aux_path);
            target_locale = strrchr(aux_path, '/');
            target_locale++;

Патч минутой позже пришлю.

Created attachment 2044 [details]
Исправление переполнения буфера

Исправление переполнения буфера при перестройке каталогов в
/var/lib/scrollkeeper.

Created attachment 2045 [details]
Более развёрнутый патч

Другой вариант патча, написанный в соотвтетствии с man-страницой realpath.
Пробная сборка и запуск падения scrollkeeper-update не выявил.

Блеск. С учётом s/No enough/Not enough/ внесено в -alt6.

Created attachment 2047 [details]
Кумулятивный патч (segmantation+buffer)

Данный патч содержит в себе как исправления overflow buffer, так и segmentation
fault. Дополнительно вычистил всё от PATHLEN, заменил printf на sk_message,
заменил все жёсткие строки на выделение буферов. Прошу ОЧЕНЬ внимательно
проверить код! У меня всё работает, не вылетает. Внимание на выделение/очистку
памяти!

Чтобы патч увидели.

Для этой программы такой патч не нужен: слишком большой и содержит ошибки.

Всё может быть, я не идеал. А ошибки в чём? Потому как локальная тестовая сборка
с 0.3.14-alt4 у меня работает.

Created attachment 2052 [details]
scrollkeeper-0.3.14-alt-fixes.patch

Proposed fix.

ldv, посмотрел Ваш патч. Думаю, что только удаление из database.c макроса
PATHLEN приведёт к невозможности сборки программы (вряд ли подхватится из
uninstall.c). Кстати, "слишком большой" - не такой уж и серьёзный аргумент. :)

Не, вру, пропустил при чтении строку. Всё нормально.

(In reply to comment #16)
> Created an attachment (id=2052) [edit]
> scrollkeeper-0.3.14-alt-fixes.patch
> 
> Proposed fix.
А есть какие-то возражения против использования pathconf? Магическое число глаз
режет.

Created attachment 2062 [details]
Use PATH_MAX instead of a specific length

Ещё один подход к снаряду, теперь вместо 4096 используется PATH_MAX. Остаётся
последний вопрос - лично я пока не понимаю, почему аргумент количества символов
в strncat всегда один и тот же, хотя места в path с каждой итерацией становится
меньше.

Comment on attachment 2062 [details]
Use PATH_MAX instead of a specific length

Нерабочий патч, сборка не находит макрос PATH_MAX.

(In reply to comment #20)
> Created an attachment (id=2062) [edit]
> Use PATH_MAX instead of a specific length
> 
> Ещё один подход к снаряду, теперь вместо 4096 используется PATH_MAX. Остаётся
> последний вопрос - лично я пока не понимаю, почему аргумент количества символов
> в strncat всегда один и тот же, хотя места в path с каждой итерацией становится
> меньше.

Да, сейчас поглядел в man, надо указывать PATH_MAX-strlen(path)-1. Просто есть
ещё snprintf, где подобный параметр указывает полную длину буфера, на этом,
видно, мы и споткнулись.

Да, для PATH_MAX нужно добавить куда надо
#include <limits.h>

(In reply to comment #21)
> (From update of attachment 2062 [details] [edit])
> Нерабочий патч, сборка не находит макрос PATH_MAX.
> 

Надо строку
#include <limits.h>
перенести из
libs/database.c
в
libs/scrollkeeper.h.in
А из последнего выбросить упоминание о PATHLEN вообще.

Created attachment 2063 [details]
Без PATH_LEN

Может, такой вариант подойдёт?

Comment on attachment 2063 [details]
Без PATH_LEN

Во-первых, патч слишком брутален, вовсе необязательно делать всеобщую замену
PATHLEN на PATH_MAX, к тому же в разных местах по-разному.
Во-вторых, патч избыточен: например, scrollkeeper.h является генератом и
патченью не подлежит.
В целом - просьба обходиться более компактными патчами, затрагивающими минимум
строк существующего кода.

Created attachment 2065 [details]
Use PATH_MAX instead of a specific length - fixed

Патч ушёл в -alt7

(In reply to comment #25)
> (From update of attachment 2063 [details] [edit])
> Во-первых, патч слишком брутален, вовсе необязательно делать всеобщую замену
> PATHLEN на PATH_MAX, к тому же в разных местах по-разному.
Как бы такая половинная замена не выплыла боком, ибо в одном месте PATHLEN, в
другом PATH_MAX. Я не считаю это брутальностью. Хм, почему-то патч с динамически
выделяемой памятью таковым не показался.
> Во-вторых, патч избыточен: например, scrollkeeper.h является генератом и
> патченью не подлежит.
Данный файл идёт и в tar.gz, поэтому решил на всякий случай и его тронуть.
> В целом - просьба обходиться более компактными патчами, затрагивающими минимум
> строк существующего кода.
Постараюсь.

Снова вопрос: в твоём последнем патче в командах strncat не учтено твоё же
пожелание в качестве ограничителя брать ОСТАТОК свободного места, а не полную
ёмкость буфера. В моём это есть.

(In reply to comment #27)
> (In reply to comment #25)
> > (From update of attachment 2063 [details] [edit] [edit])
> > Во-первых, патч слишком брутален, вовсе необязательно делать всеобщую замену
> > PATHLEN на PATH_MAX, к тому же в разных местах по-разному.
> Как бы такая половинная замена не выплыла боком, ибо в одном месте PATHLEN, в
> другом PATH_MAX.
Нет, везде должен использоваться PATHLEN (желательно один и тот же), но он
определяется через PATH_MAX.

> Я не считаю это брутальностью. Хм, почему-то патч с динамически
> выделяемой памятью таковым не показался.
Показался :) Мне кажется, в данном случае динамически выделяемая память - это
overkill.

> > Во-вторых, патч избыточен: например, scrollkeeper.h является генератом и
> > патченью не подлежит.
> Данный файл идёт и в tar.gz, поэтому решил на всякий случай и его тронуть.
В tar.gz идут и файлы configure, но тем не менее во многих наших пакетах
делается autoreconf, в результате configure патчить бесполезно.

> > В целом - просьба обходиться более компактными патчами, затрагивающими минимум
> > строк существующего кода.
> Постараюсь.
Прошу прощения, возможно, был слишком резок. Не обижайтесь :)

(In reply to comment #28)
> Снова вопрос: в твоём последнем патче в командах strncat не учтено твоё же
> пожелание в качестве ограничителя брать ОСТАТОК свободного места, а не полную
> ёмкость буфера. В моём это есть.
Упс, а вот это уже мой косяк. Конечно же, должен быть остаток. Значит, будет
-alt8...

Возясь с GLEW, наткнулся на проблему с определением и переопределением макросов
и последующим их уничтожением. Хотел сохранить параметр и потом восстановить
его. Поэтому написал так:

#define MY_PAR <нечто>
...
#define SAVE_PAR MY_PAR
...далее MY_PAR менялся, что-то происходило
#define MY_PAR SAVE_PAR
#undef SAVE_PAR

На самом деле получилось, что из-за #undef макрос MY_PAR становился просто
SAVE_PAR, а не той величиной, что хотел туда загнать. Поэтому стараюсь обходить
 стороной цепочки определений.

Да, вот ещё: в libs/database.c в sk_mkdir_with_parents перед завершением надо
ещё обратно освобождать память, на которую указывает pathcopy из-за команды strdup.

P.S. Я и не обиделся. Надо же на чём-то учиться.

В моём варианте патча pathconf не используется, стало быть и освобождать память
не нужно. Насчёт цепочек переопределений макросов - в данном случае (!) я
предпочитаю точечность патча, поскольку она как раз гарантирует бОльшую
надёжность в итоге. Везде и так используется PATHLEN, и PATHLEN всё равно
задаётся некоторой цифрой; почему бы тогда эту цифру не взять из другого макроса?
А вот баловаться с #undefine'ами и повторными #define'ами действительно опасно.

Created attachment 2070 [details]
Respect the path length in strncat's

Пробуем закрыть баг ещё раз. У меня всё отработало, но на краевом случае (с
длиной пути ~4096) я сборку не проверял.