Handle mod keys correctly

[iacore]

Need to first install QubesOS/qubes-gui-daemon#98, otherwise mod keys may not work.

To fix:

• set correct mode of focus events, since some applications like xfce4 terminal rely on that to ignore window manager hot keys (see https://stackoverflow.com/questions/15270420/why-xgrabkey-generates-extra-focus-out-and-focus-in-events) not as important
• events arrive out-of-order. No such bug found.
• duplicate RawKeyPress & RawKeyRelease for every key. Probably keyboard driver issue. Fixed in qubes-guid.

[iacore]

X's documentation is so bad that I did this with trail and error.

[marmarek]

otherwise mod keys may not work.

This is unacceptable. If introducing incompatible protocol change (which, again, I'm not sure if is really necessary), it must be correctly handled. Generally, semantic of existing messages should not change; if really necessary, new messages can be introduced.

[iacore]

It's that keys like Ctrl and Alt are pressed and released twice without this patch. Most applications work fine.

[DemiMarie]

otherwise mod keys may not work.

This is unacceptable. If introducing incompatible protocol change (which, again, I'm not sure if is really necessary), it must be correctly handled. Generally, semantic of existing messages should not change; if really necessary, new messages can be introduced.

One approach is for the daemon to send a “I support XYZ” message when it connects. The agent will then know that a new daemon is running and it can ignore the old events.

[DemiMarie]

The GUI daemon can send a message that states, “XYZ is supported, use it if you know about it”.

[marmarek]

It's called protocol version (minor part) in the handshake.

[DemiMarie]

It's called protocol version (minor part) in the handshake.

I thought the agent told the daemon its version, but not the reverse. So the agent still needs some way to know whether to expect the new messages.

[marmarek]

It's called protocol version (minor part) in the handshake.

I thought the agent told the daemon its version, but not the reverse. So the agent still needs some way to know whether to expect the new messages.

Ah, you're right, that's the other way around.

[DemiMarie]

Nit: missing newline at EOF

[iacore]

Weird thing: sometimes, when starting up my arch linux AppVM, it says "sticky key enabled". Maybe this pressing shift a lot of times?

Not an issue in terms of usability, just a bit annoying to see the notification. It only happens at VM start up.

[DemiMarie]

Weird thing: sometimes, when starting up my arch linux AppVM, it says "sticky key enabled". Maybe this pressing shift a lot of times?

Does this happen with a patched GUI daemon?

[iacore]

Weird thing: sometimes, when starting up my arch linux AppVM, it says "sticky key enabled". Maybe this pressing shift a lot of times?

Does this happen with a patched GUI daemon?

I do not remember it happening lately. Maybe it's fixed, maybe not. It only happened "sometimes" before the patch.

One way to fix it properly is to maybe disable sticky key in XFCE in AppVM. I don't know how to do that easily.

[DemiMarie]

@locriacyber: I think this can be split into a cleanup patch (which does not change behavior) and a patch that changes behavior conditionally on the GUI daemon supporting a new protocol version.

@marmarek: I am inclined to make the changes in protocol 1.4 as minimal as possible, as everyone needs to have updated to a daemon that supports it before we can ship an agent that uses it. Therefore, I think this should be delayed until no earlier than protocol version 1.5.

[iacore]

I'll try if I can make this backwards compatible. Can I use Zig to write the patch? I'm already reaching my limit with C and 2 features. It's easier for me to rewrite the whole thing in C.

Also, it's better to negotiate protocol version.

QubesOS/qubes-gui-daemon#98 (comment)

[DemiMarie]

I'll try if I can make this backwards compatible. Can I use Zig to write the patch? C is not as flexible.

I doubt @marmarek or myself know any Zig. The long term plan is to rewrite the entire daemon in Rust, but that will also involve switching from X11 to Wayland.

[iacore]

Interfacing Zig with C is easy. It's possible to replace a single C function with Zig. Rust and the borrow checker makes it harder (all the unsafe you have to write). zig translate-c is amazing to use if you want to translate a C function to Zig to patch it.

Still, if we have to rewrite the whole thing, why don't we use existing solution? (you mentioned one written in Rust before)

[DemiMarie]

Interfacing Zig with C is easy. It's possible to replace a single C function with Zig. Rust and the borrow checker makes it harder (all the unsafe you have to write). zig translate-c is amazing to use if you want to translate a C function to Zig to patch it.

The problem is that neither @marmarek (to the best of my knowledge) nor myself know any Zig. We cannot accept patches we cannot review properly.

Still, if we have to rewrite the whole thing, why don't we use existing solution? (you mentioned one written in Rust before)

That one does not exist yet, and it will be quite a while before anyone gets around to implementing it.

[iacore]

Personally, I think Zig is a good choice at incrementally replacing C. It's
Zig documentation

The most important different of Zig from C is the pointer types are more fine grained and way harder to make mistakes

• *T single pointer, no pointer arithmetic, no null. ?*T is nullable pointer.
• ?T is either T (by-value) or nothing. Use val.? or if expression to get the wrapped value out.
• []T is slice of T with length. In C this would be two arguments: char * s, int len.
• [6]T is fixed sized array. No implicit coersion from this type to []T.
• E!T or !T (inferred E) is either error E or T, where E is errorset.
• Errorset is like enum in C and cannot cross ABI boundary. Better than using -1, -2 to denote error.
• union(enum) is tagged enum

*T in C is ?[*]T in Zig (nullable pointer to array or a single item). You should never use this type unless you are dealing with C. Only [*]T allow you to do unchecked pointer arithmetic.

Otherwise, the memory model is the same as C. But apart from memory model, Zig's compiler is strict.

• matching enum is exhaustive
• enum and is its own type. It's still integer in memory though (you can choose the size too).
• Few casts are implicit (only from literal to concrete type). All other casts look like @truncate.

As for memory allocation, there is no global malloc/free. Instead you pass std.mem.Allocator to any function that needs to allocate stuff.

Good of Zig than Rust is, I can control memory usage precisely, and it's easier to interface with C than C.
Bad of Zig than Rust: I still forget to release memory sometimes with Zig. There is no support for RAII.

[meithecatte]

What is Zig's story for temporal memory safety? If I understand correctly, Zig doesn't have any way of avoiding use-after-free other than "be careful".

[DemiMarie]

What is Zig's story for temporal memory safety? If I understand correctly, Zig doesn't have any way of avoiding use-after-free other than "be careful".

Yup. It is not possible to prevent use-after-free without some form of lifetime analysis or a garbage collector. If one wants safe references into tagged unions and a guarantee of no iterator invalidation, then I believe one basically needs either the full Rust borrow checker or something even more complex, like full dependent types and hand-written (but machine-checked) proofs of correct API usage.

[marmarek]

From the description above (which is my only knowledge of this language) Zig sounds appealing. But I don't want to introduce yet another language to the code base, especially one that isn't known to our team (and likely most contributors).

[iacore]

What is Zig's story for temporal memory safety? If I understand correctly, Zig doesn't have any way of avoiding use-after-free other than "be careful".

From https://ziglearn.org/chapter-2/:

The Zig standard library also has a general purpose allocator. This is a safe allocator which can prevent double-free, use-after-free and can detect leaks. Safety checks and thread safety can be turned off via its configuration struct (left empty below).

It still doesn't prevent race r/w cross thread.

Yup. It is not possible to prevent use-after-free without some form of lifetime analysis or a garbage collector. If one wants safe references into tagged unions and a guarantee of no iterator invalidation, then I believe one basically needs either the full Rust borrow checker or something even more complex, like full dependent types and hand-written (but machine-checked) proofs of correct API usage.

Check out compile-time use analysis in Koka. Data types that is proven to be acyclic do not invoke GC. You can write code with "weak" reference (lookup maybe fail) and not use GC at all. Dependent types has nothing to do with memory usage. You still need some kind of GC, or never use cyclic reference. There is also HVM which clones your function before calling it (it never shares anything).

[iacore]

@marmarek Can I try to patch gui-agent in Zig? This way you can see how well it integrates with existing code. Working with C programs that contains all definition in a single file has been difficult for me.

[DemiMarie]

@marmarek Can I try to patch gui-agent in Zig? This way you can see how well it integrates with existing code. Working with C programs that contains all definition in a single file has been difficult for me.

Would splitting the C into multiple files help? I at least cannot review Zig as I do not know the language, and contributions that cannot be reviewed cannot be accepted.

[iacore]

I'll try

[iacore]

Superseded by #169