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 737586 - Lock screen hangs on password check if the password typed too quickly
Lock screen hangs on password check if the password typed too quickly
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: lock-screen
3.14.x
Other Linux
: Normal major
: ---
Assigned To: GDM maintainers
GDM maintainers
Depends on:
Blocks:
 
 
Reported: 2014-09-29 13:08 UTC by Balló György
Modified: 2015-04-07 20:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
authPrompt: Fix race condition during login (1.99 KB, patch)
2015-03-27 10:21 UTC, Shivam Mishra
needs-work Details | Review
authPrompt: Fix hang if user types password really fast (1.99 KB, patch)
2015-04-07 19:57 UTC, Shivam Mishra
committed Details | Review

Description Balló György 2014-09-29 13:08:44 UTC
Steps to reproduce:
1. Set a very short password for your user account (e.g. one character, but can be reproduced with longer passwords also).
2. Log in into GNOME, and lock the screen. The GDM lock screen appears, and the monitor turns off.
3. Type your password quickly, and hit enter without waiting for the monitor turned on or the password field become visible.
4. It would be expected that GDM would unlock the screen now, but instead of that it just hangs on password check.
5. Hit on the cancel button and try again typing your password a bit slower. Now GDM unlocks the screen as expected.

Package version: gdm 3.14.0 (can be reproduced with 3.12.2 also)

Distribution: Arch Linux (can be reproduced also on Fedora)
Comment 1 Clément Guérin 2015-01-25 13:01:48 UTC
Can confirm with GDM 3.14.1 and a 1-letter password. Can't reproduce with a 3-letter password or more, I guess that's why it went unnoticed ;)
Comment 2 Samuel Sieb 2015-03-09 23:26:01 UTC
I run into this often with a longer password although I usually remember to wait now.  If you hit enter before it's "ready" (whatever that involves), it just hangs until you hit ESC or the cancel button.  At least the shield needs to be up and the password field needs to be fully activated, it seems to blink once.  I probably run into this easier, because I just start typing without raising the shield first and I'm usually coming back from a suspend which may slow things down a bit.

Currently gnome-shell 3.14 on Fedora 21.
Comment 3 Shivam Mishra 2015-03-27 10:21:25 UTC
Created attachment 300426 [details] [review]
authPrompt: Fix race condition during login

Currently when password is entered too quickly without lifting the
screen shield, then there is a race between gdmclient identifying the
service and authprompt trying to verify the password entry with the
service, due to which the login screen hangs. Ideally gdmclient should
identify the service first then authPrompt should try to verify the
password entry with that service but when typed in too quickly the
reverse happens.
To make the login proceed, password verification is redone after
setting _queryingService = serviceName.
Comment 4 Shivam Mishra 2015-03-27 10:25:59 UTC
(In reply to Shivam Mishra from comment #3)
> Created attachment 300426 [details] [review] [review]
> authPrompt: Fix race condition during login
> 
> Currently when password is entered too quickly without lifting the
> screen shield, then there is a race between gdmclient identifying the
> service and authprompt trying to verify the password entry with the
> service, due to which the login screen hangs. Ideally gdmclient should
> identify the service first then authPrompt should try to verify the
> password entry with that service but when typed in too quickly the
> reverse happens.
> To make the login proceed, password verification is redone after
> setting _queryingService = serviceName.

When password is typed too quickly without lifting the screen shield, then there seems to be a race condition between identifying the service name (in this case "gdm-password") done in _onAskQuestion() and verifying the password (this._userVerifier.answerQuery(this._queryingService, this._entry.text)) done after "next" signal is emitted. The 2nd one out runs the first one whereas the opposite should happen, i.e. first _onAskQuestion() should be called and set
	this._queryingService = serviceName;
then only
        this.connect('next', Lang.bind(this, function() {
                         this.updateSensitivity(false);
                         this.startSpinning();
                         if (this._queryingService) {
                             this._userVerifier.answerQuery(this._queryingService, this._entry.text);
                         } else {
                             this._preemptiveAnswer = this._entry.text;
                         }
                     }));
should be run.
To fix this, i could think of 3 solutions:
-Introduce some sort of delay before "if (this._queryingService)" during which "this._queryingService = serviceName;" would get executed in _onAskQuestion()
-Check after "this._queryingService = serviceName;" if there is an existing _preemptiveAnswer then call
	this._userVerifier.answerQuery(this._queryingService, this._preemptiveAnswer);
-Check when "next" signal is emitted, if(this._queryingService==null) then reset the login process without lifting the screen shield and let user login again

In this patch I have implemented the 2nd solution, but I am not sure if its the best one or if there is something else that could be done. I would want some review on that.
Thanks
Comment 5 Michael Catanzaro 2015-04-04 16:33:12 UTC
Ray, can you take a look at this patch when you get a chance? It's for a GSoC application.

Easiest way to reproduce the reported bug is to use a VM with low memory. If the system is swapping, it's pretty much impossible to log in until you figure out the trick that you have to wait for the Password prompt.
Comment 6 Ray Strode [halfline] 2015-04-07 13:21:51 UTC
yea of course.  thanks for looking into this, will review shortly.
Comment 7 Ray Strode [halfline] 2015-04-07 14:12:50 UTC
Review of attachment 300426 [details] [review]:

Hey thanks again for looking into this. I appreciate the investigation and code. I don't think the patch is quite right, but it's close.  The way the code is supposed to work, in the race you're talking about, is:

1) user types password very quickly and hits enter
2) "next" signal is emitted since enter key was hit
3) the "next" handler code you pasted is run
4) this._queryingService is null since the user hit enter before it got set in onAskQuestion
5) Since this._queryingService is null, the "next" handler doesn't call answerQuery but instead just stuffs what the user
typed into a variable (this._preemptiveAnswer)
6) GDM finally catchess up with the user and onAskQuestion is called
7) onAskQuestion notices this._preemptiveAnswer is set, so rather than ask the question it just calls answerQuery straight away.

In theory I think the above logic is right, it's just you've found a problem in the implementation of step 7.  The code tries to get the service name to answer from this._queryingService, which as you pointed out, doesn't get set until later in that function.  I think the easiest/smallest change is to use serviceName , not this._queryingService when doing a preemptive answer.  I think that should fix the core issue, without any other changes.

want to give it a try? But onto your patch:

::: js/gdm/authPrompt.js
@@ +196,2 @@
     _onAskQuestion: function(verifier, serviceName, question, passwordChar) {
+        if (this._preemptiveAnswer && this._queryingService) {

So I don't believe this code will ever run if you add this._queryingService to the check. You could just remove the whole block especially since you do it again now below.

@@ +205,2 @@
         this._queryingService = serviceName;
+        if (this._preemptiveAnswer) {

You've effectively moved the block down to later in function.  This way this._queryingService is set and we won't end up calling answerQuery with an unset service. That's interesting fix, and a potentially viable alternative to my proposal above of using serviceName instead of this._queryingService. 

I don't think I like this solution, as much, though, because it means _queryingService is set when a service isn't actively waiting on a query response. I think this._queryingService should only be set if there's a service waiting for an answer. 

Your solution also means this.clear() will get called.  this.clear() does two things:

1) stop the spinner that's started when the user hits enter
2) clear the entry box.

Off hand I'm not sure if we want to do those two things or not when answering preemptively.  What do you think?
Comment 8 Shivam Mishra 2015-04-07 17:44:54 UTC
Hi, thanks a lot for reviewing the patch and i totally agree with you, even I feel that my solution wasnt really a very clean one but at that time I couldnt think of anything better so i thought i should submit my patch and will improve based on reviews.

I would surely like to try what you said about using serviceName instead of this._queryingService when doing a preemptive answer, that seems to be a very clean solution but there is just one thing i am still unsure of:

Lets say there is a situation when we receive an answer before receiving the query but before we could set this answer into _preemptiveAnswer, the first "if" condition of _onAskQuestion is already executed and at that moment this._preemptiveAnswer was null and therefore we moved pass that "if" condition. Now things would go as normal, answer would be stored in _preemptiveAnswer and serviceName would be stored in _queryingService (if not rechecking for existence of _preemptiveAnswer after setting _queryingService)

Now the next time we receive a query whose answer is yet not received then this time it would consider _preemptiveAnswer to be the answer of serviceName (if using serviceName insted of _queryingService in "if" condition) whereas that answer should have been for the previous query. It seems very unlikely and hypothetical situation and i am not even sure if something like this would ever happen considering the number of instructions in both threads, but i just want to confirm it. Otherwise using serviceName seems like a lot cleaner approach.

And for your 2nd question, as of now i really dont whether this.clear() should be called or not but i would surely think on that.

Thanks
Comment 9 Ray Strode [halfline] 2015-04-07 17:53:18 UTC
Hi,

(In reply to Shivam Mishra from comment #8)
> Lets say there is a situation when we receive an answer before receiving the
> query but before we could set this answer into _preemptiveAnswer, the first
> "if" condition of _onAskQuestion is already executed
can't happen because all the code in question happens on the same thread.

> i am not even sure if something like this would ever happen considering 
> the number of instructions in both threads
All javascript code runs on one thread.
Comment 10 Shivam Mishra 2015-04-07 18:01:50 UTC
Oh I am sorry, I didnt know that.
In that case,  as you said, using serviceName instead of _queryingService seems a lot cleaner approach.
I will try that and will let you know in case of any issue.

Thanks
Comment 11 Ray Strode [halfline] 2015-04-07 19:22:32 UTC
(replying to myself)
> You've effectively moved the block down to later in function.  This way
> this._queryingService is set and we won't end up calling answerQuery with an
> unset service. That's interesting fix, and a potentially viable alternative
> to my proposal above of using serviceName instead of this._queryingService. 
> 
> I don't think I like this solution, as much, though, because it means
> _queryingService is set when a service isn't actively waiting on a query
> response. I think this._queryingService should only be set if there's a
> service waiting for an answer. 
So looking at the code closer, this doesn't seem to be a real invariant. in the normal case we keep _queryingService set even after the user answers, so I think we should behave the same way with the preemptive answer. So, i like your solution better now.  Drop the spurious hunk at the top and let's go with that.
Comment 12 Shivam Mishra 2015-04-07 19:57:34 UTC
Created attachment 301089 [details] [review]
authPrompt: Fix hang if user types password really fast

It's possible for a user to type their password so quickly
that GDM hasn't even had time to ask for the password yet,
much less have time to process the answer.
In that situation, we tuck the user response away as
_preemptiveAnswer, and pass it along to GDM when GDM is finally
ready for it.
The problem is, there's a bug in the code, where we send
null for the service name in the answer, instead of the right
service name (say "gdm-password").
This commit addresses the bug by making sure we don't pass the
answer along, until the service name is properly set in
_queryingService. To ensure that, answering query (answerQuery)
based on _preemptiveAnswer has been shifted right below
this._queryingService = serviceName;
Comment 13 Ray Strode [halfline] 2015-04-07 20:01:18 UTC
Review of attachment 301089 [details] [review]:

great thanks.
Comment 14 Ray Strode [halfline] 2015-04-07 20:03:33 UTC
Attachment 301089 [details] pushed as 6660342 - authPrompt: Fix hang if user types password really fast