Bug 31351

Summary: В copy_chroot_binaries() отсутствует проверка наличия необходимых библиотек
Product: Sisyphus Reporter: solo <solo>
Component: installer-scripts-remount-stage2Assignee: Michael Shigorin <mike>
Status: CLOSED FIXED QA Contact: qa-sisyphus
Severity: normal    
Priority: P3 CC: mike
Version: unstableKeywords: distro-blocker, patch, relnote
Hardware: all   
OS: Linux   
URL: https://lists.altlinux.org/pipermail/devel/2015-October/200270.html
Bug Depends on:    
Bug Blocks: 21111, 30940    

Description solo 2015-10-09 19:28:57 MSK
В функции copy_chroot_binaries(), определённая в /usr/sbin/install2-remount-functions (предоставляется пакетом installer-scripts-remount-stage2, см. http://git.altlinux.org/people/mike/packages/?p=installer-scripts-remount-stage2.git;a=blob;f=installer-scripts-remount-stage2/scripts/install2-remount-functions;h=c689af73bd0fb19ad22c80ca893d3b1e334962f0;hb=e119820712cab35620fd73c65bb79ddfda15daf6) отсутствует проверка наличия библиотек, необходимых для копируемых бинарников. В некоторых случаях это приводит к их неработоспособности (см. <https://lists.altlinux.org/pipermail/devel/2015-October/200270.html>)
Comment 1 Michael Shigorin 2015-10-09 21:16:26 MSK
Это, кстати, результат изменения графа зависимостей...
похоже, придётся тырить/применять код из make-initrd.
Comment 2 solo 2015-10-12 13:07:11 MSK
(В ответ на комментарий №1)
> Это, кстати, результат изменения графа зависимостей...
> похоже, придётся тырить/применять код из make-initrd.

А это идея: Похоже можно заменить вызов cp на вызов /usr/share/make-initrd/tools/put-file из make-initrd -- такой вариант обещает быть более красивым, чем то что у меня на рисовалось (см. http://git.altlinux.org/people/solo/packages/installer-scripts-remount-stage2.git?p=installer-scripts-remount-stage2.git;a=commitdiff;h=433c72492313dedde71fa783770a85b9f8eaf1bc). С учётом того, что put-file нужно будет вызывать в chroot`е алгоритм будет примерно такой:

1. Проверить, существует ли $destdir/usr/share/make-initrd/tools/put-file (если нет -- выводим предупреждение и ничего не делаем.

2. Создать временный каталог $destdir/tmp/installer.XXX

3. Смонтировать в $destdir/tmp/installer.XXX текущий корень:

mount --bind / $destdir/tmp/installer.XXX

4. Выполнить копирование необходимых файлов из $destdir в $destdir/tmp/installer.XXX, используя вызов put-file внутри chroot`а $destdir

5. Отмонтировать $destdir/tmp/installer.XXX

6. Удалить каталог $destdir/tmp/installer.XXX
Comment 3 solo 2015-10-12 15:04:05 MSK
Предлагаю такой вариант -- см. http://git.altlinux.org/people/solo/packages/installer-scripts-remount-stage2.git?p=installer-scripts-remount-stage2.git;a=commitdiff;h=93bb697665fec05f1f71bb33f9581ef7b8729aa3.

PS: chroot нужен для корректной обработки ldd.
Comment 4 solo 2015-10-13 15:41:14 MSK
(В ответ на комментарий №3)
> Предлагаю такой вариант -- см.
> http://git.altlinux.org/people/solo/packages/installer-scripts-remount-stage2.git?p=installer-scripts-remount-stage2.git;a=commitdiff;h=93bb697665fec05f1f71bb33f9581ef7b8729aa3.

Вариант работает, судя по тестам.

PS: Более красиво было бы:

1. Перенести /usr/share/make-initrd/tools/put-{file,tree} в /usr/bin.

2. Выделить в отдельный пакет (put-file, например).

3. Добавить скриптам ключ --from-chroot (что позволит избавиться от mount --bind и в chroot запускать только ldd).
Comment 5 solo 2015-10-13 15:57:42 MSK
(В ответ на комментарий №4)
> (В ответ на комментарий №3)
> > Предлагаю такой вариант -- см.
> > http://git.altlinux.org/people/solo/packages/installer-scripts-remount-stage2.git?p=installer-scripts-remount-stage2.git;a=commitdiff;h=93bb697665fec05f1f71bb33f9581ef7b8729aa3.
> 
> Вариант работает, судя по тестам.

Тестировал: http://git.altlinux.org/people/solo/packages/installer-scripts-remount-stage2.git?p=installer-scripts-remount-stage2.git;a=commit;h=dffdf4874b933e93a6f0e9fd13899bc2a6622edc
Comment 6 Repository Robot 2015-10-13 16:16:15 MSK
installer-scripts-remount-stage2-0.5.2-alt2 -> sisyphus:

* Tue Oct 13 2015 Aleksey Avdeev <solo@altlinux> 0.5.2-alt2
- Add copying the libraries necessary for binaries copied (ALT#31351).
  To copy using a script /usr/share/make-initrd/tools/put-file
  belonging to an installable distribution.
Comment 7 Michael Shigorin 2015-10-13 17:14:14 MSK
Возможно, блокер bug 30940 стоило снять, хотя баг как таковой оставался:

cryptsetup - utility to setup a encrypted disks with LUKS support
* Thu Oct 08 2015 Gleb F-Malinovskiy <glebfm@altlinux> 1.6.8-alt2
- Replaced libpwquality with libpasswdqc.
* Tue Sep 08 2015 Alexey Shabalin <shaba@altlinux> 1.6.8-alt1

По части залитого пакета 0.5.2: Лёш, большая человеческая просьба пакеты моего авторства заливать всё же по согласованию, т.к. патч в текущем виде я так и не принял; прокомментирую здесь:

 copy_chroot_binaries() {
+       if [ ! -x "$destdir$PUTFILE" ]; then
+               echo "remount: file does not exist or is not available for execution: $destdir$PUTFILE" >&2
+               return
+       fi

Здесь ты теоретически ломаешь работу функции без веского повода вместо graceful degradation, когда при отсутствии более совершенного инструмента выбирается менее совершенный (другое дело, что в нынешнем установленном альтовом чруте с ядром make-initrd будет почти обязательно).

Этот кусок, вообще говоря, ломает логику и при добавлении -f это может выйти боком на ровном месте в некоторых случаях:

        if [ -f "$destdir/etc/lvm/lvm.conf" ]; then
-               mkdir -p /etc/lvm &&
-               cp -p "$destdir/etc/lvm/lvm.conf" /etc/lvm
+               echo "remount: copying /etc/lvm/lvm.conf" >&2
+               chroot "$destdir" "$PUTFILE" "$workdir" "/etc/lvm/lvm.conf" ||:
        fi

(ты потерял &&, а зачем именно для lvm.conf использовать put-file -- я лично так пока и не понял)

+       umount "$binddir"
+       rm -rf "$binddir"
 }
 
Вместо rm -rf напрашивается rmdir -- ты ведь перед монтированием делал пустой каталог :)

Т.е. вроде и без блокеров, а в угловых ситуациях разработки и применения ты нечаянно заложил несколько дополнительных мин и этого делать не следовало.
Comment 8 solo 2015-10-13 17:36:02 MSK
(В ответ на комментарий №7)
> Возможно, блокер bug 30940 стоило снять, хотя баг как таковой оставался:
> 
> cryptsetup - utility to setup a encrypted disks with LUKS support
> * Thu Oct 08 2015 Gleb F-Malinovskiy <glebfm@altlinux> 1.6.8-alt2
> - Replaced libpwquality with libpasswdqc.
> * Tue Sep 08 2015 Alexey Shabalin <shaba@altlinux> 1.6.8-alt1
> 
> По части залитого пакета 0.5.2: Лёш, большая человеческая просьба пакеты моего
> авторства заливать всё же по согласованию, т.к. патч в текущем виде я так и не
> принял;

  Поторопился, приношу извения.

> прокомментирую здесь:
> 
>  copy_chroot_binaries() {
> +       if [ ! -x "$destdir$PUTFILE" ]; then
> +               echo "remount: file does not exist or is not available for
> execution: $destdir$PUTFILE" >&2
> +               return
> +       fi
> 
> Здесь ты теоретически ломаешь работу функции без веского повода вместо graceful
> degradation, когда при отсутствии более совершенного инструмента выбирается
> менее совершенный (другое дело, что в нынешнем установленном альтовом чруте с
> ядром make-initrd будет почти обязательно).
> 
> Этот кусок, вообще говоря, ломает логику и при добавлении -f это может выйти
> боком на ровном месте в некоторых случаях:
> 
>         if [ -f "$destdir/etc/lvm/lvm.conf" ]; then
> -               mkdir -p /etc/lvm &&
> -               cp -p "$destdir/etc/lvm/lvm.conf" /etc/lvm
> +               echo "remount: copying /etc/lvm/lvm.conf" >&2
> +               chroot "$destdir" "$PUTFILE" "$workdir" "/etc/lvm/lvm.conf" ||:
>         fi
> 
> (ты потерял &&, а зачем именно для lvm.conf использовать put-file -- я лично
> так пока и не понял)

  put-file корректно обрабатывает случай, если /etc/lvm/lvm.conf симлинк, cp (без -L) -- ловил за копированием симлинка.

> 
> +       umount "$binddir"
> +       rm -rf "$binddir"
>  }
> 
> Вместо rm -rf напрашивается rmdir -- ты ведь перед монтированием делал пустой
> каталог :)
> 
> Т.е. вроде и без блокеров, а в угловых ситуациях разработки и применения ты
> нечаянно заложил несколько дополнительных мин и этого делать не следовало.

OK, Сейчас доработаю.

PS: Доработку пакетом, или патчем?
Comment 9 solo 2015-10-13 18:04:01 MSK
(В ответ на комментарий №8)
> (В ответ на комментарий №7)
...
> > Этот кусок, вообще говоря, ломает логику и при добавлении -f это может выйти
> > боком на ровном месте в некоторых случаях:
> > 
> >         if [ -f "$destdir/etc/lvm/lvm.conf" ]; then
> > -               mkdir -p /etc/lvm &&
> > -               cp -p "$destdir/etc/lvm/lvm.conf" /etc/lvm
> > +               echo "remount: copying /etc/lvm/lvm.conf" >&2
> > +               chroot "$destdir" "$PUTFILE" "$workdir" "/etc/lvm/lvm.conf" ||:
> >         fi
> > 
> > (ты потерял &&, а зачем именно для lvm.conf использовать put-file -- я лично
> > так пока и не понял)

  Не понял, о добавлении какого -f идет речь. В остальном, смены логики не вижу:

Было: Попытка копирования (cp) выполнялась только если /etc/lvm создан или существует -- т. к. в противном случаи mkdir вернёт ошибку и && не даст выполнить cp.

Стало: Если по каким-то причинам put-file не может создать /etc/lvm -- он обламывается, возвращая ошибку.

Т. е. в обоих вариантах в случаи невозможности создать /etc/lvm имеем отсуьсвие копирования $destdir/etc/lvm/lvm.conf и признак ошибки в $?.

Прошу указать на слом логики.

> 
>   put-file корректно обрабатывает случай, если /etc/lvm/lvm.conf симлинк, cp
> (без -L) -- ловил за копированием симлинка.
Comment 10 solo 2015-10-13 18:46:23 MSK
(В ответ на комментарий №8)
> (В ответ на комментарий №7)
...
> > 
> > Т.е. вроде и без блокеров, а в угловых ситуациях разработки и применения ты
> > нечаянно заложил несколько дополнительных мин и этого делать не следовало.
> 
> OK, Сейчас доработаю.

  Доработанный вариант -- http://git.altlinux.org/people/solo/packages/installer-scripts-remount-stage2.git?p=installer-scripts-remount-stage2.git;a=commitdiff;h=260ce0d154daa337a01f20c7b11d0065a5e10771

> 
> PS: Доработку пакетом, или патчем?
Comment 11 solo 2015-10-13 21:35:46 MSK
Прошу смотреть http://git.altlinux.org/tasks/151373/.
Comment 12 Michael Shigorin 2015-10-14 17:25:19 MSK
(В ответ на комментарий №9)
> Не понял, о добавлении какого -f идет речь.

#!/bin/sh -f (у нас порой производится устрожение скриптов, а тут последствия при таком стали бы зависимы от ситуации времени выполнения -- т.е. тестирование изменения могло бы не показать добавленной проблемы, которая потом могла вылезти фатальным сбоем инсталятора в ситуации, где ранее работало).

(В ответ на комментарий №10)
> PS: Доработку пакетом, или патчем?
Обычно бывает удобней сперва отсмотреть и если что -- поправить (--amend), но на глаз теперь почти порядок; единственно что не понял: зачем делать put-file применительно к /etc/lvm/lvm.conf, оно же не должно быть бинарником -- или это ты механически переделал на put-file и второе вхождение cp? :)

PPS: по результатам теста будто работает; на всякий обрати внимание -- regular-server.iso у меня с таким пакетом встал (при этом /mnt/destination/sbin/lvm и прочие нужные скрипту бинари были скопированы в /sbin инсталятора и не имели выпадений по библиотекам, судя по ldd), но не смог загрузиться с lvm-ного корня: initrd не выполнил vgchange -ay, после ручного подъёма в lvm, монтирования в /root и exec /root/sbin/init установка загрузилась.
Comment 13 Michael Shigorin 2015-10-14 17:25:34 MSK
В смысле закидывай.
Comment 14 solo 2015-10-14 18:47:49 MSK
(В ответ на комментарий №12)
> (В ответ на комментарий №9)
> > Не понял, о добавлении какого -f идет речь.
> 
> #!/bin/sh -f (у нас порой производится устрожение скриптов, а тут последствия
> при таком стали бы зависимы от ситуации времени выполнения -- т.е. тестирование
> изменения могло бы не показать добавленной проблемы, которая потом могла
> вылезти фатальным сбоем инсталятора в ситуации, где ранее работало).

  Судя по man sh, при sh -f поведение измениться не должно, т. к. нет автодополнение имён файлов (что -f отключает) не используется. А вот при sh -e поведение изменено: сделана маскировка кода возврата при ошибках (через ||:), т. к. требуется корректно отмонтировать примонтированное.

> 
> (В ответ на комментарий №10)
> > PS: Доработку пакетом, или патчем?
> Обычно бывает удобней сперва отсмотреть и если что -- поправить (--amend), но
> на глаз теперь почти порядок; единственно что не понял: зачем делать put-file
> применительно к /etc/lvm/lvm.conf, оно же не должно быть бинарником -- или это
> ты механически переделал на put-file и второе вхождение cp? :)

  Не, специально:

1. Мне нравиться как put-file обрабатывает симлинки -- при копировании структуры вида <симлинк> -> <файл> она воссоздаётся.

2. Можно выкинуть mkdir.

3. Однообразие.

  На самом деле, если положиться исключительно на put-file, то суть copy_chroot_binaries() можно сократить до:

copy_chroot_binaries() {
	binddir="$(mktemp -d "$destdir/tmp/copy_chroot_binaries.XXXXXXXXX")"
	workdir="${binddir#$destdir}"
	mount --bind / "$binddir"

	chroot "$destdir" "$PUTFILE" "$workdir" "$MDADM" "$LVM" "$CRYPTSETUP" /etc/lvm/lvm.conf ||:

	umount "$binddir"
	rmdir "$binddir"
}

т. к. put-file практически все необходимые проверки выполняет.

> 
> PPS: по результатам теста будто работает; на всякий обрати внимание --
> regular-server.iso у меня с таким пакетом встал (при этом
> /mnt/destination/sbin/lvm и прочие нужные скрипту бинари были скопированы в
> /sbin инсталятора и не имели выпадений по библиотекам, судя по ldd), но не смог
> загрузиться с lvm-ного корня: initrd не выполнил vgchange -ay, после ручного
> подъёма в lvm, монтирования в /root и exec /root/sbin/init установка
> загрузилась.

Для информации: у меня в системе поставленной с того инсталлятора, который я сейчас мучаю (пакет с предлагаемыми правками там всегда) LVM автоматом стартует. Но там у меня systemd (по ТЗ он там нужен).


(В ответ на комментарий №13)
> В смысле закидывай.

OK. Закинул.

Но после -- нашёл ляп... Прошу смотреть очередной фикс http://git.altlinux.org/people/solo/packages/installer-scripts-remount-stage2.git?p=installer-scripts-remount-stage2.git;a=commitdiff;h=7c26c2ff301304cc0988590fad5990918be2d612. Там возвращается потерянная скобка:

-       if -n "$useputfile" ]; then
+       if [ -n "$useputfile" ]; then
Comment 15 solo 2015-10-14 18:57:53 MSK
(В ответ на комментарий №14)
...
> 
> Но после -- нашёл ляп... Прошу смотреть очередной фикс
> http://git.altlinux.org/people/solo/packages/installer-scripts-remount-stage2.git?p=installer-scripts-remount-stage2.git;a=commitdiff;h=7c26c2ff301304cc0988590fad5990918be2d612.
> Там возвращается потерянная скобка:
> 
> -       if -n "$useputfile" ]; then
> +       if [ -n "$useputfile" ]; then

Прошу пропуска для installer-scripts-remount-stage2-0.5.4-alt1 (см. http://git.altlinux.org/tasks/151413/logs/events.1.1.log).
Comment 16 Michael Shigorin 2015-10-14 19:04:10 MSK
(В ответ на комментарий №14)
> > > Не понял, о добавлении какого -f идет речь.
> > #!/bin/sh -f

В смысле -e, конечно.

> > ты механически переделал на put-file и второе вхождение cp? :)
>   Не, специально:

А.

> На самом деле, если положиться исключительно на put-file, то суть
> copy_chroot_binaries() можно сократить до:

У нас пока есть mkinitrd (хоть уже и не поддерживается, но в некоторых армовых образах задействовалось).

> (В ответ на комментарий №13)
> > В смысле закидывай.
> OK. Закинул. Но после -- нашёл ляп... Прошу смотреть очередной фикс

Что самое смешное, я его тоже проморгал, читая дифф.  Закидывай, конечно.