Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent recursive includes of keymap components #389

Closed
wants to merge 2 commits into from

Conversation

wismill
Copy link
Member

@wismill wismill commented Oct 25, 2023

Prevent recursive includes of keymap components:

  • Add file name property to XkbFile. This allow better identification of the file being processed.
  • Add check for recursive includes of keycodes, key types, compat and symbols keymap components.
  • Add tests for recursive includes.

I would like to get the design right wrt further work on #390.

@wismill
Copy link
Member Author

wismill commented Oct 25, 2023

There seems to be memory leaks in key types code that make the test suite failed when runing with valgrind.

@wismill wismill marked this pull request as ready for review October 25, 2023 08:17
@wismill wismill requested review from whot and bluetech October 25, 2023 08:17
When there is no error the types are “stolen” and copied to the keymap.
But when there is an error, `MergeIncludedKeyTypes` just return without
“stealing” nor freeing the types.

Fixed by explicitly freeing the key types.
@wismill wismill mentioned this pull request Oct 25, 2023
@wismill wismill force-pushed the include/check-recursive branch from b69a538 to 9abe663 Compare October 25, 2023 19:07
- Add file name property to `XkbFile`. This allow better identification
  of the file being processed.
- Add check for recursive includes of keycodes, key types, compat and
  symbols keymap components.
- Add tests for recursive includes.
@wismill wismill force-pushed the include/check-recursive branch from 9abe663 to 50e2939 Compare October 25, 2023 19:11
@wismill
Copy link
Member Author

wismill commented Oct 25, 2023

Fixed memory leak. I found some other leaks and grouped them in #391.

This PR will need to be rebase once #391 is merged.

@bluetech
Copy link
Member

A simpler and arguably more robust method for detecting include loops is to have an include depth limit. Have you considered it?

@wismill
Copy link
Member Author

wismill commented Oct 26, 2023

@bluetech I did, but then I thought it would be cleaner to do an exact check that:

  • avoids cycling completely: check occurs before loading the recursive file.
  • provides useful log message.

I guess this issue is unlikely enough that we could just do as you propose. I have no idea what is the depth threshold in xkeyboard-config. I guess 15 is a reasonable threshold, with plenty of margin for keymaps in the wild.

@wismill
Copy link
Member Author

wismill commented Oct 26, 2023

Created an alternative design in #392.

@wismill wismill closed this Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants