Skip to content

Conversation

@PekingSpades
Copy link

Summary

Fix a memory corruption bug in keyCodeForChar() function in mac/TestInput/TestInput/TestInputController.m where a 64-bit pointer-sized value was being written into a 16-bit CGKeyCode variable, causing stack corruption.

Problem

The original code had a dangerous pointer/memory width mismatch:

CGKeyCode code;  // uint16_t, 16 bits
// ...
CFDictionaryGetValueIfPresent(charToCodeDict, charStr, (const void **)&code)

CFDictionaryGetValueIfPresent writes a pointer-sized value (64 bits on modern systems) to the memory location provided. Casting &code (a pointer to a 16-bit variable) to const void ** causes the function to write 8 bytes into a 2-byte memory location, corrupting 6 bytes of adjacent stack memory.

This is undefined behavior and can lead to:

  • Stack corruption
  • Random crashes
  • Incorrect return values

Solution

Use an intermediate pointer-sized variable to safely receive the dictionary value, then cast to CGKeyCode:

const void *value = NULL;
CGKeyCode code;
if (CFDictionaryGetValueIfPresent(charToCodeDict, charStr, &value)) {
    code = (CGKeyCode)(uintptr_t)value;
} else {
    code = UINT16_MAX;
}

References

Apple Documentation

64-bit Porting Best Practices

may related issues: #3084 #894 #1005 #1072 #1143 #11674 #11673 #11057

@github-project-automation github-project-automation bot moved this to Todo in Keyman Dec 31, 2025
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label Dec 31, 2025
@keymanapp-test-bot
Copy link

User Test Results

Test specification and instructions

ERROR: user tests have not yet been defined

@keymanapp-test-bot keymanapp-test-bot bot added this to the A19S19 milestone Dec 31, 2025
@keyman-server
Copy link
Collaborator

This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR.

@mcdurdin mcdurdin requested a review from sgschantz December 31, 2025 22:25
@mcdurdin
Copy link
Member

mcdurdin commented Jan 1, 2026

Thank you for your PR @PekingSpades. I am curious, what is your purpose in submitting this PR?

This is in an internal app that does not get distributed, and is used for test purposes only. Clearly you have found a bug (and it appears, using AI to generate the PR?), but it is of low significance and zero impact to end users, so your PR does not provide a lot of benefit.

@PekingSpades
Copy link
Author

Hi @mcdurdin

Thanks for the feedback. I’d like to clarify that I am a human developer, not a bot. To be honest, I have no incentive to waste my own computing power or rack up expensive token bills just to blindly scan repositories for low-impact bugs—especially since current AI models aren't actually capable of detecting this specific logic flaw on their own. I discovered this issue manually while working deeply with robotgo and realized that this same error has been propagated across several major GUI automation libraries, like nut.js, through various forks and ports over the years.

While I did use AI to help draft the PR description for better clarity, the discovery and the fix itself came from my own investigation. My motivation for submitting this is quite simple: I currently work at an AI company focusing on model training, so I am acutely aware of how critical high-quality, "clean" code is for the future of development. I submitted this PR because I respect your work and don't want AI models to learn from and further spread incorrect code patterns. I believe keeping our open-source ecosystem's "source material" clean benefits everyone, regardless of whether the tool is for internal testing or production.

Best regards,
@PekingSpades

@mcdurdin
Copy link
Member

mcdurdin commented Jan 2, 2026

Thanks @PekingSpades for your thoughtful reply -- understand and agree that good clean code across the open source ecosystem is pretty important for the future (but perhaps an insurmountable challenge?). I will let @sgschantz review this as it is his area, but from my perspective, this does look like a good cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

user-test-missing User tests have not yet been defined for the PR

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants