Fix crash when an element of SymbolsBody is empty #423

chiro · 2023-12-27T00:51:49Z

An empty element is allowed in SymbolsBody definition, so the following keymap is gramatically correct.

xkb_keymap {
  xkb_symbols "sym" {
    key <SPC> {, [Space] };
  };
};

However, the current parser crashes with the keymap due to null pointer access.
This change fixes the crash.

wismill · 2023-12-27T06:49:01Z

Thank you for the bug report.

I confirm this syntax is accepted by xkbcomp and is interpreted as if there was no leading comma.

But I think this is an oversight, because this syntax is useless. As Ivan Pascal wrote, the intended syntax would rather be:

key <SPC> {[], [Space] };

which parses correctly in xkbcommon.

Anyway, a segmentation fault is definitly not acceptable!

wismill

@chiro The fix LGTM, but the test needs some fixes, please look at my suggestions.

@whot @bluetech this PR would allow to parse input currently accepted by xkbcomp, but I think it really should not be accepted there either.

test/data/keymaps/empty-symbol-decl.xkb

test/filecomp.c

chiro · 2023-12-27T11:53:48Z

@wismill Thanks for the prompt review! I fixed the test. PTAL again.

I agree that the parser rule is not useful. However changing it introduces backward incompatibility, so I decided to fix the crash for now.

bluetech

Thanks @chiro, LGTM.

Agree it's not worth breaking backward compat over.

I'll leave it to @wismill to make the final decision.

bluetech · 2023-12-30T11:44:55Z

test/data/keymaps/empty-symbol-decl.xkb

+    xkb_types     { include "complete" };
+    xkb_compat    { include "complete" };
+    xkb_symbols "sym" {
+        key <SPC> { , [Space] };


Suggested change

key <SPC> { , [Space] };

// This dubious syntax is accepted by xkbcomp so we accept it for

// backward compatibility. The preferred way to write it is:

// key <SPC> { [], [Space] };

key <SPC> { , [Space] };

Actually, these syntaxes are not equivalent. Using X11 xkbcomp, I get that

key <SPC> { [Space] }; key <SPC> { , [exclam] };

is equivalent to

key <SPC> { [exclam] };

whereas

key <SPC> { [Space] }; key <SPC> { [], [exclam] };

is equivalent to

key <SPC> { symbols[Group1]= [ Space ], symbols[Group2]= [ exclam ] };

That’s why I wrote it was probably an oversight.

Thanks for the clarification. then, is there any other changes needed?

@chiro I wonder: where did you encounter this syntax in the first place? Unless there is a real-world proof, I think we should fix just the segfault, but disallow this syntax. It is useless per se and confusing. It is not used in xkeyboard-config.

I do not see to make key <SPC> { , [exclam] }; equivalent to key <SPC> { [], [exclam] }; either: XKB has C-style, so it would not be intuitive.

Fully agree with @wismill here - if this is somewhere in the xkb maps we should fix it in xkeyboard-config but allowing this syntax seems like the wrong approach.

@wismill It was not found in an actual in-use layout file. It's detected in testing. As far as I know, there is no real-world use case of this syntax. I have no objection to disallowing this syntax.

Just to clarify, I couldn't find any real-world use case, but it doesn't mean there is no use case in the world. That's why I made the CL just to fix the crash.

@chiro we are grateful for uncovering the segfault. This is a nasty bug, and it would not occur to me to test this syntax. Well done!

I am curious, could you share how you stumble on this? Maybe we could then improve our test suite further.

Now, given that:

AFAIK the syntax is useless and undocumented;

xkeyboard-config compiles just fine with xkbcommon;

no one opened a ticket before.

then I really doubt there are use cases in the world. While we strive to have maximum compatibility with X11, I think this is one edge case we should not support. Probably worth a comment, at least in the code.

It was reported as a potential security issue to the project I'm working on (Actually it was not because only layout files in xkeyboard-config are used). I don't know the details of how it was discovered, but it should be by fuzzing.

If it's not used in any existing xkeyboard-config files, can we just make it return a parser error? That seems like the best option here - making this intentionally backwards-compatible means we have to support it forever when there seems to be little point to it.

Thanks for the comment (and the reaction)! I changed this PR not to allow the syntax. Could you review it again? Thanks.

test/data/keymaps/empty-symbol-decl.xkb

wismill · 2024-01-10T21:28:15Z

test/data/keymaps/empty-symbol-decl.xkb

+    xkb_types     { include "complete" };
+    xkb_compat    { include "complete" };
+    xkb_symbols "sym" {
+        key <SPC> { , [Space] };


@chiro I wonder: where did you encounter this syntax in the first place? Unless there is a real-world proof, I think we should fix just the segfault, but disallow this syntax. It is useless per se and confusing. It is not used in xkeyboard-config.

I do not see to make key <SPC> { , [exclam] }; equivalent to key <SPC> { [], [exclam] }; either: XKB has C-style, so it would not be intuitive.

An empty element is allowed in SymbolsBody definition, so the following keymap is gramatically correct. ``` xkb_keymap { ... xkb_symbols "sym" { key <SPC> {, [Space] }; }; }; ``` However, the current parser crashes with the keymap due to null pointer access. This change fixes it by changing the parser not to allow it.

whot

LGTM now, @wismill can you merge it if you're happy with it? Thanks.

wismill · 2024-02-05T08:42:14Z

Thanks @chiro !

chiro · 2024-02-06T01:27:21Z

Thank you all for reviewing!

wismill added bug Indicates an unexpected problem or unintended behavior compile-keymap Indicates a need for improvements or additions to keymap compilation labels Dec 27, 2023

wismill added this to the 1.7.0 milestone Dec 27, 2023

wismill requested changes Dec 27, 2023

View reviewed changes

test/data/keymaps/empty-symbol-decl.xkb Outdated Show resolved Hide resolved

test/filecomp.c Show resolved Hide resolved

bluetech approved these changes Dec 30, 2023

View reviewed changes

wismill reviewed Jan 10, 2024

View reviewed changes

chiro force-pushed the fix_crash branch from 2c7c814 to a0dd92b Compare January 27, 2024 14:04

whot approved these changes Jan 28, 2024

View reviewed changes

wismill merged commit efdb05d into xkbcommon:master Feb 5, 2024
4 checks passed

chiro deleted the fix_crash branch February 6, 2024 01:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash when an element of SymbolsBody is empty #423

Fix crash when an element of SymbolsBody is empty #423

chiro commented Dec 27, 2023

wismill commented Dec 27, 2023

wismill left a comment

chiro commented Dec 27, 2023

bluetech left a comment

bluetech Dec 30, 2023

wismill Dec 30, 2023

chiro Jan 2, 2024

wismill Jan 10, 2024

whot Jan 12, 2024

chiro Jan 12, 2024

wismill Jan 12, 2024

chiro Jan 12, 2024

whot Jan 25, 2024

chiro Jan 27, 2024

wismill Jan 10, 2024

whot left a comment

wismill commented Feb 5, 2024

chiro commented Feb 6, 2024

-        key <SPC> { , [Space] };
+        // This dubious syntax is accepted by xkbcomp so we accept it for
+        // backward compatibility. The preferred way to write it is:
+        // key <SPC> { [], [Space] };
+        key <SPC> { , [Space] };

Fix crash when an element of SymbolsBody is empty #423

Fix crash when an element of SymbolsBody is empty #423

Conversation

chiro commented Dec 27, 2023

wismill commented Dec 27, 2023

wismill left a comment

Choose a reason for hiding this comment

chiro commented Dec 27, 2023

bluetech left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whot left a comment

Choose a reason for hiding this comment

wismill commented Feb 5, 2024

chiro commented Feb 6, 2024