Right-click menu cancels lock about i3lock HOT 33 CLOSED

in my opinion this #35 and #115 are a major bug, that should definitely be fixed.

from i3lock.

Thanks for the contributions to the discussion, everyone.

Here are my thoughts on various aspects that were brought up:

1. While it is true that most of our individual programs are useful with window managers other than i3, I think we shouldn’t use that as an argument to block tighter integration between them, as long as we don’t entirely break non-i3 window manager use-cases by that. If we make focus handling not work for some WM, that’s a price I’m willing to pay for making it work in the i3 case. If focus handling must be broken for some subset of people (and I’m not yet convinced that’s the case), we should favor the subset which contains the i3 users.
2. xscreensaver’s focus changing approach does seem superior than doing nothing: it will strictly improve the experience for some users, while for those who are content with the current locking behavior will not change anything — i3lock already messes up the focus in its current form (i3 focus will be restored where the pointer is, not where keyboard focus was).
3. The main criticism regarding xscreensaver’s focus changing is that it is changing focus directly (using the SetInputFocus X11 request). So why don’t we adopt the approach, but implement focus changing correctly, i.e. with the _NET_ACTIVE_WINDOW ClientMessage?
4. I’d argue that philosophically, the user made their intent clear by starting i3lock: the screen should be locked. If a context menu needs to close because of that, then so be it. It’s not like the user loses data or anything. This interpretation is consistent with how commands within i3 behave: they modify state if required to fulfill the user’s intent.
5. The bug pointed out in

   i3lock/xcb.c

   Lines 203 to 207 in 698204a

   	if (!redrawn &&
   	(tries % 100) == 0 &&
   	(clock() - start) > 250000) {
   	redraw_screen();
   	redrawn = true;

   regarding its use of CPU time instead of wall-clock time is valid and should be fixed.

from i3lock.

Wow, I still can't believe the progression of this issue. Thank you both for fixing this. If we'll ever meet in real life, I'll buy you a beer.

from i3lock.

Yes, that’s how X11 works. Context menus grab the pointer, so i3lock can’t grab it.

I don’t think we can do anything about this.

from i3lock.

I'm astonished of your logic. This really blows my mind.

from i3lock.

Maybe just as some additional information, the reason why it takes a few seconds is because we actually repeatedly try to grab the pointer before giving up (rather than just giving up right away). I think this is even the recommended way of doing it in the XCB docs somewhere.

from i3lock.

Oh, okay, sure. Had no idea. But maybe it would be better -- for the purposes of a lock screen -- if it just gave up right away, so the user doesn't lock, walk away, and then have the screen unlock.

from i3lock.

We don't give up right away because the existing grab might be very short-lived and we don't want the lock to fail like every other time or so.

I wouldn't necessarily be against introducing an indicator that stays red until the screen is actually locked or not mapping the i3lock window until then or something, so the user would have correct visual feedback. What do you think, @stapelberg ?

from i3lock.

I think we should show the unlock indicator with a text like “locking…”

from i3lock.

Is this the same issue that lets dmenu to circumvent the lock (if timed properly)?

from i3lock.

Yes.

from i3lock.

Wouldn't it be better to reopen it and mark it as wontfix. Many are not aware of this and it's nowhere mentioned, that i3lock fails to lock if it can't grab the pointer. Also it's a quite common thing having a context menu opened.

Yes, that’s how X11 works. Context menus grab the pointer, so i3lock can’t grab it.

@stapelberg So, why does every other screen locker not fail?

from i3lock.

Marking an issue as won'tfix still means closing it. And I would argue the common part: context menus are, in their nature, transient. It means you are currently trying to do something. I don't know why in that same moment you'd want to lock and I certainly don't think it's difficult or a problem to close it before locking.

from i3lock.

So, why does every other screen locker not fail?

Which screen locker specifically did you try that doesn’t exhibit this problem? Also, in which application did you open a context menu?

from i3lock.

Which screen locker specifically did you try that doesn’t exhibit this problem? Also, in which application did you open a context menu?

I just installed xscreensaver now and picked a random one (here Viligance). It works there flawlessly.

Also, in which application did you open a context menu?

Tested in Firefox and Konsole. This might be enough to prove, that it's not a specific problem to a specific software.

from i3lock.

The relevant xscreensaver code:

 AGAIN:

  for (i = 0; i < retries; i++)
    {
      XSync (si->dpy, False);
      kstatus = grab_kbd (si, window, screen_no);
      if (kstatus == GrabSuccess)
        break;

      /* else, wait a second and try to grab again. */
      sleep (1);
    }

  if (kstatus != GrabSuccess)
    {
      fprintf (stderr, "%s: couldn't grab keyboard!  (%s)\n",
               blurb(), grab_string(kstatus));

      if (! focus_fuckus)
        {
          focus_fuckus = True;
          nuke_focus (si, screen_no);
          goto AGAIN;
        }
    }

So after a few retries of trying to get a keyboard grab, xscreensaver calls nuke_focus which does this:

static void
nuke_focus (saver_info *si, int screen_no)
{
  saver_preferences *p = &si->prefs;
  Window focus = 0;
  int rev = 0;

  XGetInputFocus (si->dpy, &focus, &rev);

  if (p->verbose_p)
    {
      char w[255], r[255];

      if      (focus == PointerRoot) strcpy (w, "PointerRoot");
      else if (focus == None)        strcpy (w, "None");
      else    sprintf (w, "0x%lx", (unsigned long) focus);

      if      (rev == RevertToParent)      strcpy (r, "RevertToParent");
      else if (rev == RevertToPointerRoot) strcpy (r, "RevertToPointerRoot");
      else if (rev == RevertToNone)        strcpy (r, "RevertToNone");
      else    sprintf (r, "0x%x", rev);

      fprintf (stderr, "%s: %d: removing focus from %s / %s.\n",
               blurb(), screen_no, w, r);
    }

  XSetInputFocus (si->dpy, None, RevertToNone, CurrentTime);
  XSync (si->dpy, False);
}

It does another few retries with this and then does(!) give up, much like i3lock. »Nuking focus«, as they call it, is a way to deal with it, but it does something a screen locker shouldn't be doing (granted, this is due to the poor design of X11 here). I think it's fair to say that i3lock's approach to tell the user that locking is in progress and failing if it doesn't work is not better or worse than xscreensaver's approach. A user who cares about the security should be expected to wait the couple hundred of milliseconds until i3lock confirms that the lock has been established.

If you care about this »feature«, it'd be simple enough to write a tool that »nukes focus« and chain it with i3lock. Problem solved, so my stance here is unchanged in that there is nothing for i3lock to do.

from i3lock.

Addendum: if you write such a tool, make sure to return focus once the chained (i3lock) process exits.

from i3lock.

Also note that xscreensaver's approach actually doesn't work for all context menus. For example, the context menu opened by thunar does not close when losing focus like that.

from i3lock.

Here's the simple skeleton for such a program:

#include <stdlib.h>
#include <stdio.h>
#include <stdbool.h>
#include <string.h>
#include <err.h>
#include <X11/Xlib.h>

int main(int argc, char *argv[]) {
    Display *display = XOpenDisplay(NULL);
    if (!display) {
        fprintf(stderr, "Could not open X connection!\n");
        exit(EXIT_FAILURE);
    }

    fprintf(stdout, "Querying current input focus...\n");
    Window focus;
    int rev;
    XGetInputFocus(display, &focus, &rev);

    fprintf(stdout, "Removing input focus...\n");
    XSetInputFocus(display, None, RevertToNone, CurrentTime);
    XSync(display, false);

    fprintf(stdout, "Running command \"%s\"\n", command);
    // TODO Run a command passed as an argument in a blocking way (see "man 3 exec")
    
    fprintf(stdout, "Restoring input focus...\n");
    // This could fail if the window has closed in the meantime, but what are 'ya gonna do?
    // The call will simply fail, no harm done -- except that it might leave your window manager in a
    // surprising status.
    XSetInputFocus(display, focus, rev, CurrentTime);

    XCloseDisplay(display);
}

Compile (requires x11) and run as sleep 5; /path/to/compiled-binary, quickly open a context menu in firefox and see it magically disappear a few seconds later.

from i3lock.

Thanks @Airblader for your thorough research. I think you pointed out very good, that X11 handling is quite cumbersome. From a dev perspective, I fully agree with you.

»Nuking focus«, as they call it, is a way to deal with it, but it does something a screen locker shouldn't be doing (granted, this is due to the poor design of X11 here).

Could you please elaborate this? Why shouldn't a screensaver do this? IMHO a screensaver should do everything it can do, no matter what.

Please look at this from a user perspective.

The GitHub page presents i3lock as improved screen locker, so as a user, I've got the expectation, that it actually locks my screen and does not fail on a simple hurdle. Saying that the user is currently in a transient action, is no excuse to not lock the screen.

Either you should make the user aware of this (mentioning it in the docs and providing a workaround) or simply fix this behavior. All other screenlockers work this way and as a user I've got that expectation, that a screen locker does not fail on missing pointer focus.

from i3lock.

Could you please elaborate this? Why shouldn't a screensaver do this?

Because manually setting input focus bypasses the specifications on how to change focus in a managed environment which are in place for a good reason. i3lock is part of the i3 suite, but runs independent from the window manager used, and we simply don't know how either the window manager or the window will react when input is violently taken from it. In other words, it's just wrong to do this – IMHO.

Yes, as a consequence locking will succeed (although even then not in all cases, see above), but when the user comes back and unlocks the screen, they will find focus to be in a different state than they left it – this certainly goes against the user's expectations as well.

If it was at least a 100% reliable method, I'd be somewhat open to consider doing it in i3lock as well, but given that it isn't, I don't see a point in adding this kind of hack to i3lock.

IMHO a screensaver should do everything it can do, no matter what.

I slightly disagree; the screen locker should try to lock the screen, but do everything it can do to inform the user about the correct status and not let them mistakenly believe their screen is locked if it isn't.

i3lock used to have a problem here, but we've since fixed this by displaying a message to the user that locking is in progress. Locking the screen can always fail, so if your screen locker tells you that the screen has not yet been locked, stick around and wait for it to tell you that it has indeed been locked.

Saying that the user is currently in a transient action, is no excuse to not lock the screen.

I agree that this should be the world we live in, but it's just not. X is simply broken here and there's nothing we can do about it. Screen locking under X has a variety of pitfalls, which, if you're interested, you can read up on in the FAQ of xscreensaver.

Either you should make the user aware of this (mentioning it in the docs and providing a workaround)

The user is informed via the on-screen message (which is far better than being buried in docs most people will never read) and there's now a workaround, so both of those criteria are fulfilled. :-)

as a user I've got that expectation, that a screen locker does not fail on missing pointer focus.

You've gotten used to the wrong expectation; all (X11) screen lockers bail when they cannot grab the keyboard, even xscreensaver. Or, since you are referring to »pointer focus«, perhaps you mean the pointer grab instead? There is indeed a difference here that xscreensaver views the pointer grab as optional, while i3lock requires it. However, this has nothing to do with the issue being discussed here since it's about failing to acquire the keyboard grab.

from i3lock.

Addendum: we actually know for a fact that i3 isn't reacting all too nicely to an application setting input focus manually, see i3/i3#2722. Granted, we do want to fix that in i3, but it demonstrates what I said above.

from i3lock.

Yes, as a consequence locking will succeed (although even then not in all cases, see above), but when the user comes back and unlocks the screen, they will find focus to be in a different state than they left it

So you're valuing input focus preservation higher than locking the screen?

Addendum: we actually know for a fact that i3 isn't reacting all too nicely to an application setting input focus manually, see i3/i3#2722. Granted, we do want to fix that in i3, but it demonstrates what I said above.

I admit, the way with least resistance is probably to fix it by myself. I still can't support your viewpoint, but I understand now, why you don't want to have such behavior.

i3lock used to have a problem here, but we've since fixed this by displaying a message to the user that locking is in progress. Locking the screen can always fail, so if your screen locker tells you that the screen has not yet been locked, stick around and wait for it to tell you that it has indeed been locked.

But I have to chime in here again:

I don't know what you're talking about. The code tries 10000x to grab the input focus (which takes roughly 1 sec) and then shows a the Lock Failed status. During input grabbing there is no indication, that the screen is not yet locked and it looks like it locked properly.

I know there is #88, which should have changed this behavior, but the message triggered by STATE_AUTH_LOCK never gets printed.

[bebe:~] % sleep 3 && i3lock --debug
[i3lock-debug] device = 3
[i3lock-debug] found Xinerama screen: 1920 x 1080 at 0 x 0
[i3lock-debug] found Xinerama screen: 1920 x 1080 at 1920 x 0
[i3lock-debug] scaling_factor is 1, physical diameter is 190 px
[i3lock-debug] redraw_screen(unlock_state = 0, auth_state = 4)  <- only redrawn, when FAILED
[i3lock-debug] scaling_factor is 1, physical diameter is 190 px
i3lock: Cannot grab pointer/keyboard

The problem is the hardcoded clock value, which has to exceed 250k, but on my clock it takes less than 200k.

At least this is a point, where an improvement could be done.

from i3lock.

So you're valuing input focus preservation higher than locking the screen?

I think this exaggerates my statement. We are not talking about i3lock randomly unlocking the screen here, but about i3lock failing to start. Any program ever written has the chance to fail to start.

The code tries 10000x to grab the input focus

Which is what the Xlib / xcb docs, at some point, recommended. :-)

The problem is the hardcoded clock value, which has to exceed 250k, but on my clock it takes less than 200k.

The idea here was to avoid flickering in the case that locking is indeed successful, which will be the vast majority of cases. I still think that idea is solid, and arguing over the exact value is cumbersome since there are just many different machines out there.

from i3lock.

Either you should make the user aware of this (mentioning it in the docs and providing a workaround) or simply fix this behavior. All other screenlockers work this way and as a user I've got that expectation, that a screen locker does not fail on missing pointer focus.

Could you then please mention the problem in the docs until there is a solution?

I think this exaggerates my statement. We are not talking about i3lock randomly unlocking the screen here, but about i3lock failing to start. Any program ever written has the chance to fail to start.

Usually a screen-locker are used to prevent others to use your machine, so they can not steal your data or install unicorn-wallpapers. Sometimes users forget to lock their screen, so the computer sits there and after a few minutes of inactivity it will lock it self.

imho a screen-locker should be able to lock in every situation. Not only if the user forces it. Because sometimes the user is not available to check if the locking is fine/completely forgot about locking.

I totally agree that any program has the chance to fail while starting. But in this case you have to minimize the chances of failing. Otherwise i3lock misses the goal of being a lock-screen.

from i3lock.

Could you then please mention the problem in the docs until there is a solution?

Feel free to send a PR that documents this in a good spot. However, just to be clear, a solution will never come because X11 just isn't being developed like that anymore. The solution in this case would be Wayland (for all I know, anyway.)

Sometimes users forget to lock their screen, so the computer sits there and after a few minutes of inactivity it will lock it self.

And in the vast majority of cases i3lock will work just fine. Even xscreensaver has cases where it fails. If you are absolutely paranoid about security (which, to be clear, is a positive thing), get in the habit of locking your machine explicitly. Walking away from an unlocked machine because you trust it'll lock itself after some time is a flawed thought in its very nature.

If you find yourself just forgetting to lock it too often, it's time to think about additional steps to take, independent of the screen locker you are using. Coupling it with the bluetooth connection to your phone, webcam inactivity, you name it – there's many additional things you can do, but they are not in the scope of i3lock.

imho a screen-locker should be able to lock in every situation.

Yes, in an ideal world this is true, as I've said before. We're really just running around in circles now.

I've given (most of) the code above to write a hack that does the focus trick. For reasons I've outlined I don't want to put this into i3lock itself, but you are free to run it, so the solution you are asking for is already there, you just need a second binary. That isn't so bad, is it?

from i3lock.

I've actually thought of a way to secure your system even in the case that you forget to lock it: use i3lock's exit code and chain it with a command that force-shutdowns your machine. That way if the screen cannot be locked you do not leave your system vulnerable.

Granted, it is extreme, but if you want to make no compromise with security under X, that's probably the best way to go. This goes for xscreensaver as well, by the way.

from i3lock.

I've only given you a way to avoid the typical X11 pitfalls if you choose that security is the most important factor. As I said, it is extreme and I'd personally never go down that route (but I also don't leave context menus open).

Let's please keep in mind that xscreensaver also has this problem, for example with thunar. This is not specific to i3lock. In fact, some Google employees wrote their own screen locker and recommend that very same approach[1] (just FYI).

X is broken, period. Let's please accept that. So you need to decide on your own how far you go and how much you prioritize security. If you expect your screen locker to do everything in its power to establish security for the case that you left your machine unattended, then excluding shutting down the machine in the extreme case from those possibilities is a rather arbitrary choice, IMHO. For my personal machine that'd be insane, for a work laptop with sensitive information it might be a real option.

[1] https://github.com/google/xsecurelock#security-design

from i3lock.

i3lock already messes up the focus in its current form (i3 focus will be restored where the pointer is, not where keyboard focus was).

A different window being focused is quite a bit different from leaving the user with an unusable i3 session, which is a potential outcome of using XSetInputFocus at the moment: if the client's window disappears after input focus has been set to None, and there is no other window on the screen, the user has no way of focusing anything and all keyboard access is broken.

So why don’t we adopt the approach, but implement focus changing correctly, i.e. with the _NET_ACTIVE_WINDOW ClientMessage?

Because you cannot use that request to set the focus to None, but only to other windows. In the i3 case even only to managed windows. So perhaps we can use the window identified by _NET_SUPPORTING_WM_CHECK for this, which opens even more holes of this feature (for other window managers).

As for the rest, here are my final thoughts: I think it's uncanny that I am strictly against this. When it comes to security, it seems ridiculous to me to complain that i3lock doesn't secure one's system because of context menus, yet the only mechanism that truly protects against this is written off as insane. This solution is a half-baked one because i3lock is now taking a middle ground between comfort and security. Unfortunately, when it comes to security, there is no middle ground.

Granted, this basically applies to the current form of i3lock. But here is my issue with putting in this approach which we know to be flawed from the get go:

Users now might just learn the habit of leaving context menus open because »it works« – until it doesn't.

And once your machine is both unlocked and unattended at the same time, for all you know, all of your data has been compromised already. But unlike just losing it, you lost it to someone.

Given all the interest surrounding this, I can't wait for the PR to come in.

from i3lock.

if the client's window disappears after input focus has been set to None, and there is no other window on the screen, the user has no way of focusing anything and all keyboard access is broken.

Sorry, I don’t quite follow — at which point would input focus be set to None, specifically?

from i3lock.

Currently, never. But that's what this approach that I dislike so much is all about: setting input focus to None in order to steal focus from the application with the context menu in the hope that it'll close said menu as a result.

from i3lock.

My current approach is to set the input focus to the i3lock window (with RevertToPointerRoot), not to None.

from i3lock.

That should have the same broken effect once i3lock exits, I think. But maybe it's just broken for RevertToNone. Either way, that's just a bug in i3 anyway and not my argument against this change. We need to fix this in i3 regardless.

from i3lock.