Bug 34721 - Every Xvnc session leaks a file descriptor
Summary: Every Xvnc session leaks a file descriptor
Status: CLOSED FIXED
Alias: None
Product: Sisyphus
Classification: Development
Component: lightdm (show other bugs)
Version: unstable
Hardware: all Linux
: P3 major
Assignee: Arseny Maslennikov
QA Contact: qa-sisyphus
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-27 22:00 MSK by Arseny Maslennikov
Modified: 2018-03-28 10:17 MSK (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arseny Maslennikov 2018-03-27 22:00:09 MSK
Если включить поддержку VNC-сервера в lightdm, то по каждому подключению деймон открывает файловый дескриптор (начиная с 13), смотрящий в сокет TCP-соединения, который (в рамках жизненного цикла деймона) не закрывается больше никогда.

Ошибка воспроизводится всегда.

В случае, когда сеанс завершается по инициативе X-сервера, утекает ещё и TCP-четвёрка (соединение никогда не выходит из CLOSE_WAIT).

http://git.altlinux.org/people/arseny/packages/?p=lightdm.git;a=commit;h=7830aa609bb4f0ec037fa8e540dd7a96b85868b0
Comment 1 Arseny Maslennikov 2018-03-27 22:22:19 MSK
Fixed in Sisyphus by task #203066
Comment 2 Ivan Zakharyaschev 2018-03-28 00:13:47 MSK
(In reply to comment #1)
> Fixed in Sisyphus by task #203066

Спасибо!

gear-store-tags неудачно там сохранил значения: patches/advanced не включает последние изменения из alt18, в результате сгененрированный diff их отменяет.

Это можно видеть в патче на session-child.c в https://packages.altlinux.org/en/Sisyphus/srpms/lightdm/patches/lightdm-1.16.7-alt19-advanced.patch , строка 479:

@@ -811,7 +848,8 @@ session_child_run (int argc, char **argv)
         if (error)
             g_printerr ("Error removing X authority: %s\n", error->message);
         g_clear_error (&error);
-        /* Ignore this error, don't exit, continue closing the session. */
+        if (!result)
+            _exit (EXIT_FAILURE);
     }
 
     /* Close the Console Kit session */

Тут нужно придерживаться правило, чтобы patches/advanced всегда был наследником patches/alt-fixes, а также прошлого значения patches/advanced. К сожаленью, gear это не проверяет (аналогично сборочному коммиту), хотя смысл похожий.

Нужно переделать...
Comment 3 Ivan Zakharyaschev 2018-03-28 00:21:58 MSK
Ещё лучше не совмещать несвязанные whitespace changes и исправления в одном коммите.

Советую также предложить upstream-у на https://github.com/CanonicalLtd/lightdm/pulls?q=is%3Apr+is%3Aclosed ; мою поправку смёрджили.
Comment 4 Ivan Zakharyaschev 2018-03-28 00:46:52 MSK
(In reply to comment #3)

> Советую также предложить upstream-у на
> https://github.com/CanonicalLtd/lightdm/pulls?q=is%3Apr+is%3Aclosed ; мою
> поправку смёрджили.

Я, кстати, ради простоты создания pull request-а сделал свой патч не очередным коммитом в линейно тянувшейся ветке patches/alt-fixes, а взял за основу upstream-ный коммит соответствующего релиза, а потом свой коммит смёрджил ("параллельно") в patches/alt-fixes.

gear-схеме с diff-ами не противоречит.
Comment 5 Arseny Maslennikov 2018-03-28 02:55:21 MSK
(In reply to comment #3)
> Ещё лучше не совмещать несвязанные whitespace changes и исправления в одном
> коммите.
> 
> Советую также предложить upstream-у на
> https://github.com/CanonicalLtd/lightdm/pulls?q=is%3Apr+is%3Aclosed ; мою
> поправку смёрджили.

Взглянул на глазок. Где-то значительно после 1.16 код, связанный с vnc-сеансами, был существенно переработан; у них это, наверное, уже исправлено.
Однако я 1.26 не собирал и ручаться не буду. :)
Comment 6 Arseny Maslennikov 2018-03-28 03:03:24 MSK
(In reply to comment #2)
> (In reply to comment #1)
> > Fixed in Sisyphus by task #203066
> 
> Спасибо!
> 
> gear-store-tags неудачно там сохранил значения: patches/advanced не включает
> последние изменения из alt18, в результате сгененрированный diff их отменяет.
> 
> Это можно видеть в патче на session-child.c в
> https://packages.altlinux.org/en/Sisyphus/srpms/lightdm/patches/lightdm-1.16.7-alt19-advanced.patch
> , строка 479:
> 
> @@ -811,7 +848,8 @@ session_child_run (int argc, char **argv)
>          if (error)
>              g_printerr ("Error removing X authority: %s\n", error->message);
>          g_clear_error (&error);
> -        /* Ignore this error, don't exit, continue closing the session. */
> +        if (!result)
> +            _exit (EXIT_FAILURE);
>      }
> 
>      /* Close the Console Kit session */
> 
> Тут нужно придерживаться правило, чтобы patches/advanced всегда был наследником
> patches/alt-fixes, а также прошлого значения patches/advanced. К сожаленью,
> gear это не проверяет (аналогично сборочному коммиту), хотя смысл похожий.
> 
> Нужно переделать...

Торопыжка был голодный, проглотил утюг холодный...

Переделал в alt20.
Глазами проверил генерируемый %nvr-advanced.patch, вроде всё хорошо.
Кстати, так и не понятно до сих пор, зачем аж две ветки с патчами?
Comment 7 Arseny Maslennikov 2018-03-28 03:13:43 MSK
(In reply to comment #5)
> Взглянул на глазок. Где-то значительно после 1.16 код, связанный с
> vnc-сеансами, был существенно переработан; у них это, наверное, уже исправлено.

https://github.com/CanonicalLtd/lightdm/blob/master/src/seat-xvnc.c#L112

> Однако я 1.26 не собирал и ручаться не буду. :)
Comment 8 Ivan Zakharyaschev 2018-03-28 10:16:16 MSK
(In reply to comment #6)

> Переделал в alt20.
> Глазами проверил генерируемый %nvr-advanced.patch, вроде всё хорошо.

Спасибо!

> Кстати, так и не понятно до сих пор, зачем аж две ветки с патчами?

Не знаю, по-моему, если и разделять, то лучше на отдельные, логически независимые ветки-патчи, а одну ветку в другую мёрджить, есди они логически зависимы.

Но по отдельной ветке на патч -- больше мороки. Я так практиковался делать в https://www.altlinux.org/%D0%97%D0%BD%D0%B0%D0%BA%D0%BE%D0%BC%D1%81%D1%82%D0%B2%D0%BE_%D1%81%D0%BE_%D1%81%D1%85%D0%B5%D0%BC%D0%BE%D0%B9_%D1%83%D0%BF%D0%B0%D0%BA%D0%BE%D0%B2%D0%BA%D0%B8_pam-pkcs11 .

Либо уж просто последовательность коммитов-патчей в одной ветке. 

Если это для удобства мейнтейнера (две ветки), то всё равно непонятно, зачем генерировать два diff-а, а не один. (Это бы от таких ошибок избавило.)
Comment 9 Ivan Zakharyaschev 2018-03-28 10:17:33 MSK
(In reply to comment #8)

> Если это для удобства мейнтейнера (две ветки), то всё равно непонятно, зачем
> генерировать два diff-а, а не один. (Это бы от таких ошибок избавило.)

Ну не избавило бы, но сделало бы их менее вероятными.