We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 3469 - Keypresses leak to the console with 2.0.5 (didn't happen with 2.0.4)
Summary: Keypresses leak to the console with 2.0.5 (didn't happen with 2.0.4)
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: events (show other bugs)
Version: 2.0.5
Hardware: ARM Linux
: P2 critical
Assignee: tvc
QA Contact: Sam Lantinga
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-25 09:52 UTC by Manuel Alfayate Corchete
Modified: 2017-01-13 04:29 UTC (History)
2 users (show)

See Also:


Attachments
enable keyboard muting (755 bytes, patch)
2016-10-31 03:13 UTC, tvc
Details | Diff
remove tty search and enable keyboard muting (3.62 KB, patch)
2017-01-07 10:06 UTC, tvc
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Alfayate Corchete 2016-10-25 09:52:30 UTC
On the Raspberry Pi platform, where SDL2 runs in console mode (using dispmanx + GLES renderer), keypresses during the game/app runtime now appear on the console. They did not appear with stable 2.0.4.
It's a critical issue because, for example, pressing the up arrow + enter inside the game/app runs commands that the user ran before launching that game/app, which could have all kind of dangerours consequences on the system.
Comment 1 Sam Lantinga 2016-10-25 16:42:49 UTC
I don't have access to the Raspberry Pi platform. Can you identify which commit caused the regression?
Comment 2 Manuel Alfayate Corchete 2016-10-26 09:08:16 UTC
I have just took a look at http://hg.libsdl.org/SDL/shortlog, and it's HUGE.
Building every "candidate" revision is impossible.
However, if you tell me what I should look for, I would be able to do a reasonabe number of builds.
I am willing to send you a Raspberry Pi, too. SDL2 is too important on the platform.
Comment 3 Philipp Wiesemann 2016-10-26 19:12:12 UTC
The patch for bug 3157 may have affected the keyboard handling.
Comment 4 Manuel Alfayate Corchete 2016-10-27 09:40:25 UTC
(In reply to Philipp Wiesemann from comment #3)
> The patch for bug 3157 may have affected the keyboard handling.

Can you indicate the revision numbers before and after the suspicious patch was merged? I am not used to this hg and I can't find out. An indication on how to fund out would be great, too.
Comment 5 Philipp Wiesemann 2016-10-27 21:38:12 UTC
The revision before the patch was applied is "eeb3847d81a6" and the revision after the patch was applied is "2b008531d078".
Comment 6 Sam Lantinga 2016-10-27 21:38:39 UTC
You can see the revision numbers here:
https://hg.libsdl.org/SDL/rev/0f79dd727ed6
Comment 7 Manuel Alfayate Corchete 2016-10-29 11:42:20 UTC
@Sam Lantiga: I have built both eeb3847d81a6 and 2b008531d078 revisions, and indeed key leakage appears JUST after 2b008531d078.
So that's the commit where it appears. Thanks Phillipp for your suggestion as it was spot-on.
Comment 8 Sam Lantinga 2016-10-31 02:21:44 UTC
tvc, can you look at this regression caused by your patch?

Thanks!
Comment 9 tvc 2016-10-31 03:13:04 UTC
Created attachment 2595 [details]
enable keyboard muting

This was a complete oversight by me, clearly I didn't do proper testing. Sorry all.

I have attached a patch that I believe will fix this bug, however I'm not able to test this personally for a couple of days. Manuel, are you able to test this in the meantime?
Comment 10 Manuel Alfayate Corchete 2016-10-31 14:20:08 UTC
me@tvc.id.au

This patch doesn't work: keycodes are still leaking to the console and now the console is corrupted after exit: keypresses send wrong codes.
Comment 11 tvc 2017-01-07 10:06:19 UTC
Created attachment 2662 [details]
remove tty search and enable keyboard muting

I believe this patch should fix it, instead of looping through all the tty's and seemingly selecting the wrong one and corrupting the console I've just made SDL open /dev/tty which is the console attached to the current process anyway.

I know I took a fair amount of time on this, however, Manuel are you still able to test this to make sure it works on your system?
Comment 12 Manuel Alfayate Corchete 2017-01-07 14:53:37 UTC
@tvc, it will be a pleasure to test this patch. SDL2 on the Pi is a fundamental part of it's operating system IMHO...
However, I have tried to apply this patch with no success:

pi@raspberrypi:~/src/SDL-2.0.5-10739 $ patch -p1 < patch.diff 
patching file src/core/linux/SDL_evdev.c
Hunk #2 succeeded at 134 with fuzz 2.
Hunk #3 FAILED at 152.
Hunk #4 FAILED at 164.
Hunk #5 succeeded at 236 (offset 4 lines).
Hunk #6 succeeded at 267 (offset 4 lines).
2 out of 6 hunks FAILED -- saving rejects to file src/core/linux/SDL_evdev.c.rej

Do I need an specific SDL2 mercurial commit or something?
Comment 13 tvc 2017-01-07 15:01:01 UTC
Sorry I should have specified, it's a patch for the latest mercurial commit.
Comment 14 Sam Lantinga 2017-01-07 17:04:11 UTC
You can grab the latest Mercurial drop from here:
http://www.libsdl.org/tmp/SDL-2.0.zip
Comment 15 Manuel Alfayate Corchete 2017-01-07 17:48:38 UTC
@tvc: This works perfectly well now. No more keypress leaks. As good as 2.0.4 was. So I'd say it's FIXED. Thanks a lot! :)
Comment 16 Sam Lantinga 2017-01-07 18:13:44 UTC
Fixed, thanks!
https://hg.libsdl.org/SDL/rev/fe9c7d01a093
Comment 17 Sam Lantinga 2017-01-08 09:31:24 UTC
tvc, your patch broke SDL on the Steam Link, where the application can run without a tty. I fixed that, but I looked more closely at what the code is doing, and I had some questions for you.
Comment 18 Sam Lantinga 2017-01-08 09:43:27 UTC
It seems like console_fd is being used for two different purposes.

It mutes the keyboard so keystrokes only generate evdev events and don't leak through to the console. In this case I think we do want the active console, whatever that is. If the application is running from somebody logged in via ssh, and it opens an evdev keyboard, we want that keyboard input to go to the SDL application and not generate key events on the shell that is logged in at the console.

Separately from that, we use the console file descriptor to do translation of key presses into characters. In this case we don't care what tty we get, we just need one that we can query the kernel translation tables from. In bug 3545 someone notes that they're running into the type < 0xf0 case, and I think that's really what we want. We want to put the tty into VC_UNICODE mode, query all the tables into userspace, reset and close the tty, and then translate those unicode values into UTF-8. That still doesn't get us dead key handling and diacritics and so forth, as you noted, but it gets basic US key translation. I haven't dug into the Linux keyboard driver to see how it handles dead keys, but I see posts from people showing that it does work, and we can probably emulate what the keyboard driver does.

Remember, SDL programs don't necessarily run from the console, and there's no reason why they can't open keyboards and mice and do the right thing in that case. :)
Comment 19 tvc 2017-01-08 10:02:39 UTC
Sorry about the Steam Link break, I didn't realise that corner case.

Well we could open /dev/console which is the first VT, switch it to K_UNICODE and read the tables. Ideally this should be done before we open /dev/tty (the current VT) for muting, as if our current VT is the first VT we wouldn't want to unmute it. I'm currently looking at the kernel sources to see how the keyboard driver handles it all, so I should have a working patch soon.
Comment 20 tvc 2017-01-08 10:11:40 UTC
(In reply to tvc from comment #19)
> Sorry about the Steam Link break, I didn't realise that corner case.
> 
> Well we could open /dev/console which is the first VT, switch it to
> K_UNICODE and read the tables. Ideally this should be done before we open
> /dev/tty (the current VT) for muting, as if our current VT is the first VT
> we wouldn't want to unmute it. I'm currently looking at the kernel sources
> to see how the keyboard driver handles it all, so I should have a working
> patch soon.

Scratch that, I was wrong, /dev/tty0 is current VT, /dev/tty1 is first VT, and /dev/tty is the VT the process was started in.
Comment 21 Sam Lantinga 2017-01-09 04:02:10 UTC
After discussion this is the correct fix.
It doesn't cover someone running the application from ssh, but in that case you can only access the virtual console if you're running as root.
Comment 22 Jools Wills 2017-01-13 03:38:57 UTC
Should https://hg.libsdl.org/SDL/rev/fe9c7d01a093 also fix this issue if applied to SDL 2.0.5 or does it rely on some additional patches ?

Running Emulation Station on Raspberry Pi directly on the framebuffer (on /dev/tty1)

I applied https://hg.libsdl.org/SDL/rev/fe9c7d01a093 to sdl 2.0.5 and the problem is still happening. Characters ending up on the console and the terminal being left in an unusable state on exit.

The latest SDL2 code does work however - so I assume I need some additional changes ?

I also applied https://hg.libsdl.org/SDL/rev/5ede976d922b but that didn't help either.
Comment 23 Jools Wills 2017-01-13 04:29:25 UTC
Unusable console on exit was fixed by cherry-picking https://hg.libsdl.org/SDL/rev/6cfccf993c5d

Also needed https://hg.libsdl.org/SDL/rev/6cfccf993c5d or else the keyboard didn't work in some parts of Emulation Station.

All seems good now. Looking forward to 2.0.6. Thanks.