Review Request 128790: Remove usage of utempter

[Oswald Buddenhagen]

> On Aug. 28, 2016, 1:41 p.m., Oswald Buddenhagen wrote:
> > uhm, and why do you think utempter is the preferred choice?
> > last time i checked, nobody was shipping konsole setgid utmp. with kdeinit, that's not even an option.
> > 
> > there are several options how to deal with this:
> > - fork()/wait() around the utempter calls, so it can't mess with the signal handling of the current process. though i seem to remmber that the addToUtmp() call actually uses the PID
> > - re-implement the libutempter calls with QProcess. that's actually how it was originally, but was changed because there were incompatible versions of utempter - but that seems like a minor concern compared to the status quo.
> > - drop utmp handling altogether, as it's been mostly superseded, first by consolekit, and now logind. however, respective bindings would have to be actually implemented, and i have no clue how things are supposed to be done. just some dbus calls?
> > -- what about non-linux systems?
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     > uhm, and why do you think utempter is the preferred choice?
>     > last time i checked, nobody was shipping konsole setgid utmp. with kdeinit, that's not even an option.
>     
>     Why is the fallback code there at all, then?
>     
>     
>     > - fork()/wait() around the utempter calls, so it can't mess with the signal handling of the current process. though i seem to remmber that the addToUtmp() call actually uses the PID
>     
>     Won't that break if another process exits at the wrong time?
>     
>     
>     > - re-implement the libutempter calls with QProcess. that's actually how it was originally, but was changed because there were incompatible versions of utempter - but that seems like a minor concern compared to the status quo.
>     
>     I looked at that as well, the issue is finding the correct path for the utempter helper.
>     
>      
>     > - drop utmp handling altogether, as it's been mostly superseded, first by consolekit, and now logind. however, respective bindings would have to be actually implemented, and i have no clue how things are supposed to be done. just some dbus calls?
>     
>     There seems to be a simple dbus call to register with logind at least. I tried looking briefly at it, but I couldn't quickly find any logind code that did utmp stuff. I didn't look very hard, though.
>     
>     
>     > -- what about non-linux systems?
>     
>     libutempter only supports Linux and FreeBSD, the fallback code seems to at least try to be compatible with other platforms.

> Why is the fallback code there at all, then?

it was there first. it will actually work if somebody runs konsole as root. which nobody does, of course.

> Won't that break if another process exits at the wrong time?

not if the parent doesn't do any messing with the signal handling. which it doesn't need to, as the defaults (and what qprocess does) are perfectly fine.
the likely pid problem remains. one could wrap only the unregistration, but then a hypothetical race between utempter registration calls and unrelated qprocess exits remains.

> I looked at that as well, the issue is finding the correct path for the utempter helper.

the configure test could run 'strings' over libutempter.so and grep for relevant patterns. ^^
or just try to divine it from the libutempter location based on typical directory structures.
the last resort would be letting the user specify it.

> I tried looking briefly at it, but I couldn't quickly find any logind code that did utmp stuff

logind doesn't do the legacy stuff any more (consolekit still did, iirc). you're supposed to use loginctl.
but i don't know whether one is actually supposed to register pseudo ttys in the first place.

> libutempter only supports Linux and FreeBSD

the two dashes were to meant to illustrate a sub-point. i.e., what about non-systemd systems?

a whole different approach would be providing an own kutempter - quite similar to kcheckpass (which is mostly redundant with pam's unix_chkpwd). or actually, to kgrantpty, which is redundant with the pt_chown helper some grantpt() implementations use.
i actually once had a todo item about that ...


- Oswald


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review98742
-----------------------------------------------------------


On Aug. 28, 2016, 1:13 p.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> -----------------------------------------------------------
> 
> (Updated Aug. 28, 2016, 1:13 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, and Thiago Macieira.
> 
> 
> Bugs: 364779
>     https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> -------
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 utempter does stuff in a way that isn't compatible with QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child process). So remove it, and rely on the fallback methods already implemented.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 3e17cac 
>   KF5PtyConfig.cmake.in 66f8c43 
>   cmake/FindUTEMPTER.cmake a3ea06a 
>   src/CMakeLists.txt caab96f 
>   src/ConfigureChecks.cmake ded08f4 
>   src/config-pty.h.cmake aaaf8d9 
>   src/kpty.cpp 15c3b81 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160828/7b98591d/attachment-0001.html>