After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 755291 - GDM does not kill pam fingerprint session if user logged in using the password
GDM does not kill pam fingerprint session if user logged in using the password
Status: RESOLVED FIXED
Product: gdm
Classification: Core
Component: general
3.16.x
Other Linux
: Normal normal
: ---
Assigned To: GDM maintainers
GDM maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-20 04:12 UTC by Vasily Khoruzhick
Modified: 2016-01-21 20:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-session-worker-drop-SIGINT-and-SIGTERM-handlers.patch (1.23 KB, patch)
2015-09-23 20:51 UTC, Vasily Khoruzhick
none Details | Review
session-worker: drop SIGINT and rework SIGTERM handler (5.04 KB, patch)
2016-01-21 20:39 UTC, Ray Strode [halfline]
committed Details | Review

Description Vasily Khoruzhick 2015-09-20 04:12:11 UTC
If fingerprint login is enabled, but user is logged in using the password, GDM does not kill pam fingerprint session. As side effect, login is delayed for 5s or so and fingerprint LED keeps blinking.
Comment 1 Ray Strode [halfline] 2015-09-20 22:31:50 UTC
probably a bug in the pam module.  I believe GDM sends SIGTERM and if the module doesn't die in 5 seconds I think it then hard kills it.
Comment 2 Vasily Khoruzhick 2015-09-21 01:32:55 UTC
I've just tried "pamtester gdm-fingerprint myuser authenticate" and I'm able to terminate process with ctrl+c.
Comment 3 Vasily Khoruzhick 2015-09-21 01:37:39 UTC
I see "Врс 20 18:35:08 anarsoul-thinkpad gdm[786]: GdmCommon: process (pid:24580, command 'gdm-session-worker [pam/gdm-fingerprint]') isn't dying after 5 seconds, now ignoring it" in journalctl.  Any ideas how to debug it?
Comment 4 Vasily Khoruzhick 2015-09-23 07:35:28 UTC
GDM never gets into session_worker_child_watch() nor session_worker_job_child_watch() for fingerprint session, and worker does not react on SIGTERM or SIGINT if I send it manually. I believe it's a bug in GDM.
Comment 5 Vasily Khoruzhick 2015-09-23 07:38:43 UTC
(In reply to Ray Strode [halfline] from comment #1)
> probably a bug in the pam module.  I believe GDM sends SIGTERM and if the
> module doesn't die in 5 seconds I think it then hard kills it.

BTW, I believe blaming code of other people before taking a look at your own is not polite. fprintd/pam_fprintd has not been changed since January 2014 and used to work with older gdm/gnome versions.
Comment 6 Ray Strode [halfline] 2015-09-23 12:14:03 UTC
(In reply to Vasily Khoruzhick from comment #3)
> Any ideas how to debug it?
attach to the process with gdb and run

(gdb) call signal (SIGTERM, SIG_DFL)

and look at the return value to see if a signal handler was set up to trap sigterm.

if so, do 

(gdb) call signal (SIGTERM, $1)

(or instead of $1 whatever temporary got assigned the return value of signal)

to reinstate that handler.  Then set a breakpoint on it. Also run

(gdb) handle SIGTERM pass nostop

to make sure gdb doesn't get in the way of SIGTERMs, then try to manually SIGTERM it.  That should fire the breakpoint.  step through it and see what it's doing.
Comment 7 Ray Strode [halfline] 2015-09-23 12:18:44 UTC
(In reply to Vasily Khoruzhick from comment #4)
> GDM never gets into session_worker_child_watch() nor
> session_worker_job_child_watch()
those functions get called when their children die.  If the processes aren't dying (which is the bug you reported !) they wouldn't get called...

> I believe it's a bug in GDM.
It's not impossible, but pretty unlikely. GDM normally sets a SIGTERM handler to quit the event loop of the session worker when it gets SIGTERM.  Thinking about it more, what's probably happening is, GDM is calling g_main_loop_quit () to quit the main loop, but the pam module is frozen up so the program never returns to the main loop.

when an attached gdb you should be able to see if that's happening by running

(gdb) thread apply all bt full
Comment 8 Ray Strode [halfline] 2015-09-23 12:22:49 UTC
(In reply to Vasily Khoruzhick from comment #5)
> BTW, I believe blaming code of other people before taking a look at your own
> is not polite. 
I haven't been impolite thus far. Note I work on both codebases (i think i've even done a release of fprintd before if i remember correctly).  Please don't judge me. you asked for help and I've given my best guesses where to start and advice on how to move forward, that's it. I'm not blaming anyone or anything.
Comment 9 Vasily Khoruzhick 2015-09-23 20:38:49 UTC
(In reply to Ray Strode [halfline] from comment #8)
> (In reply to Vasily Khoruzhick from comment #5)
> > BTW, I believe blaming code of other people before taking a look at your own
> > is not polite. 
> I haven't been impolite thus far. Note I work on both codebases (i think
> i've even done a release of fprintd before if i remember correctly).  Please
> don't judge me. you asked for help and I've given my best guesses where to
> start and advice on how to move forward, that's it. I'm not blaming anyone
> or anything.

OK, sorry, I'm taking fprint project too personally.

It appears that pam_fprint uses g_main_loop() as well as gdm-session-worker. And indeed it keeps spinning in internal (pam_fprint) main loop.
Comment 10 Vasily Khoruzhick 2015-09-23 20:51:21 UTC
(In reply to Ray Strode [halfline] from comment #7)

> Thinking about it more, what's probably happening is, GDM is calling
> g_main_loop_quit () to quit the main loop, but the pam module is frozen up
> so the program never returns to the main loop.

Looks like that's it. Attached patch fixes the issue for me
Comment 11 Vasily Khoruzhick 2015-09-23 20:51:51 UTC
Created attachment 311981 [details] [review]
0001-session-worker-drop-SIGINT-and-SIGTERM-handlers.patch
Comment 12 Ray Strode [halfline] 2015-09-23 23:08:03 UTC
Review of attachment 311981 [details] [review]:

thanks for working on this. I think this is probably okay, but it does mean that killed workers are now going to register as being terminated with a signal instead of exiting with a 0 exit code. This distinction is detected in the waitpid() call and may lead to GDM taking a different code path than it did before when the worker terminates under non-erroneous circumstances. I'll need to make sure that this change in handling won't introduce any bugs in the parent process of the worker (for instance, it could be wtmp records only get written if the worker exits successfully, don't remember off hand).  I'll investigate and report back, unless you want to do (or already did) the investigation. If it does end up being an important distinction an alternative would be to switch to a plain old signal handler with sigaction() that just calls _exit(0) (note the leading underscore which is important)
Comment 13 Vasily Khoruzhick 2015-10-29 23:15:13 UTC
Any plans to merge this one?
Comment 14 Ray Strode [halfline] 2015-10-30 15:41:15 UTC
yes, i still need to do the leg work mentioned in comment 12.  but i'd like to merge this before I do a 3.19 release.
Comment 15 Vasily Khoruzhick 2016-01-12 23:29:31 UTC
Ping?
Comment 16 Ray Strode [halfline] 2016-01-21 20:39:36 UTC
Created attachment 319524 [details] [review]
session-worker: drop SIGINT and rework SIGTERM handler

The gdm session worker sets up main loop dispatched signal
handlers for SIGINT and SIGTERM.  These handlers won't run
if the pam module is blocking, since the main loop isn't
iterating.

This commit drops the SIGINT handler, and changes the SIGTERM
handler to exit with a successful exit code, which gives us
the same effective behavior as before but will work even
if the pam module is blocked.

Small changes by Ray Strode
Comment 17 Ray Strode [halfline] 2016-01-21 20:53:07 UTC
Attachment 319524 [details] pushed as 6b85869 - session-worker: drop SIGINT and rework SIGTERM handler