Race condition / issue sending bitmaps and commands close together

[james-fowler]

reported by lmiphay in #11>
The race condition in the original tree was pretty easy to repo (haven't checked HEAD to see if its still there) - just send a image file in over the pipe and immediately send some bind key commands (unless the read from the pipe exactly matched the size expected it wouldn't be recognized as a bitmap image file).

[james-fowler]

I suspect it's still there. This might be tricky to solve completely without modifying the command protocol. There is no preamble or header in the lpbm format, so there is no foolproof way to recognize a bitmap in the pipe, since other commands could come either before or after the bitmap - potentially both.

If you only need to set the logo once, using the --logo option on the command line should avoid race conditions. It wouldn't be hard to add a "logo filename" command to load a new logo file indirectly through the pipe, which would also avoid race conditions (assuming you have the bitmap in a file).

Adding a fixed header to the .lpbm format would make it possible to fix the race conditions, although it would complicate parsing since the bitmap payload could contain newlines.

I really hate changes that break backwards compatibility. My suggestion would be to leave the current "960 byte read must be an image" logic in place - race conditions and all - and update the lpbm format to allow new stuff to work properly. Any old format .lpbm files would still work, but the new format would work without race conditions. As far as I can tell, lpbm is just a one off fixed format developed for g13.

Another option would be to support .pbm files directly, without requiring conversion to lpbm. The pbm format appears to have a header ("P4") followed by size info which would allow it to be parsed cleanly even surrounded by other commands. We could not only load a pbm file, we could load one larger than 160x48 and then pan around by specifying the top left corner. Wouldn't be super efficient, as there is no known way to internally pan or partially update the G13 frame buffer - you can only overwrite the entire thing (verified this with Logitech dev support). But you can easily manage several frames a second.

[lmiphay]

I like your suggestion of the new 'logo filename' command - particularly if it could read the file directly off the filesystem (while avoiding any security problems). It might be an issue for example if people run the daemon as root - in this case I would simply disable the logo command (log the fact of course).

And I would agree with you - don't break anything out there if it can be avoided.

[james-fowler]

What kind of security problems are you thinking about? This would open a file for read, and then only proceed as long as the file matched a valid PBM format (which is quite simple). Even as root, that seems pretty benign to me - but I could certainly be missing something. If it's really an issue, we could potentially create another command channel using domain sockets instead of named pipes, or better yet find a way to run it in daemon mode without root. I personally don't run it as root, although I don't have it configured as a daemon/system service. I also suspect anyone wanting to use one of these in a corporate environment might get some pushback from IT if it needs to run as root.

[lmiphay]

What kind of security problems are you thinking about?

logo /etc/shadow
logo /root/.ssh/id_rsa
... etc

and then:
watch the daemon log
maybe scrape the content of the G13 buffer (assuming its possible)
... etc

Checking for a particular type is good - but for me, I would still disable the command unless the daemon is running under a specified named account.

There shouldn't be any need to run it as root anyhow - I run the daemon on boot under a 'g13' account - that's how I noticed the race in the first place actually.