Skip to content
New issue

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

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

Already on GitHub? # to your account

Port fix for a segfault #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

stefan11111
Copy link
Contributor

@stefan11111 stefan11111 commented Feb 4, 2025

This fixes a segfault in the kdrive mouse driver.
I haven't managed to get tinyx to segfault, because the ability to pass mouse protocol by command line args seems to not be implemented.
Still, this doesn't mean that the segfault can't happen here too. It doesn't happen on my system because the default ps/2 protocol works fine for me. However, this might not be the case on other systems.

This code fixes an incorrect loop condition and adds a fallback for unknown mouse protocols.
Also replaces strlen with sizeof appropriately.

@stefan11111 stefan11111 force-pushed the potential-mouse-segfault branch from 381748a to 7ffe91b Compare February 5, 2025 01:59
@@ -386,8 +386,7 @@ static const KmouseProt exps2Prot = {
#define PSM_4DPLUS_ID 8

static const unsigned char ps2_init[] = {
PSMC_ENABLE_DEV,
0,
PSMC_ENABLE_DEV
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does removing these 0's fix a segfault ?

@rofl0r
Copy link

rofl0r commented Feb 5, 2025

this commit changes 4 things, but none of the changes are explained.
imagine you stumble upon this commit without having authored it and see the "==" changed to ">=" and wonder what was wrong. or why strlen was changed to sizeof. the commit message has plenty of space (in fact almost unlimited) to explain each and every change. even better would be if each change was a separate commit with an explanation.

@stefan11111
Copy link
Contributor Author

this commit changes 4 things, but none of the changes are explained.
imagine you stumble upon this commit without having authored it and see the "==" changed to ">=" and wonder what was wrong. or why strlen was changed to sizeof. the commit message has plenty of space (in fact almost unlimited) to explain each and every change. even better would be if each change was a separate commit with an explanation.

I can make separate commits. I assumed you would prefer having all changes in a single commit.

@stefan11111 stefan11111 force-pushed the potential-mouse-segfault branch 3 times, most recently from e504a2c to c344bd6 Compare February 5, 2025 09:17
This fixes an incorrect loop condtion, as it is possible to skip over NUM_PROT when incrementing
This causes a segfault in kdrive, when km->i_prot would have to loop, instead continuing to read out of bounds.
I couldn't get tinyx to also segfault, but that doesn't mean it's impossible. It might happen on different hardware.
@stefan11111 stefan11111 force-pushed the potential-mouse-segfault branch from c344bd6 to 5f3cd22 Compare February 5, 2025 09:18
@stefan11111
Copy link
Contributor Author

even better would be if each change was a separate commit with an explanation.

Done.

We know the size of the ps/2 init bytes at compile time. No need to call strlen.
@stefan11111 stefan11111 force-pushed the potential-mouse-segfault branch from a32f227 to c8fd8e5 Compare February 5, 2025 15:03
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants