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

Possible out-of-bounds reads/writes in keyboard.cpp #174

Open
ds-sloth opened this issue Jul 7, 2022 · 2 comments
Open

Possible out-of-bounds reads/writes in keyboard.cpp #174

ds-sloth opened this issue Jul 7, 2022 · 2 comments

Comments

@ds-sloth
Copy link

ds-sloth commented Jul 7, 2022

Thanks to the team for their hard work on this project and nice work on the recent implementation of UTF-8 compatibility for keyboard.cpp.

In the event that a filename was originally stored in a UTF-8 incompatible format, I believe it is possible that the first or last character of the filename will satisfy the continuation character condition (output[stringPosition] & 0xC0) == 0x80, and thus when deleting or moving the cursor left or right, the buffer will be read from (and may eventually be written to) out of bounds.

This is a very rare circumstance and I haven't had the time to produce a minimal working example of the bug, but I wanted to bring it to the developers' attention in case they would like to consider the issue.

@Epicpkmn11
Copy link
Member

I'm not sure it's possible for that to actually be an issue with file names since FAT uses UTF-16 iirc and the UTF-8 conversion is done by libfat so theoretically an invalid character should be impossible. I suppose it would probably still not be a bad idea to handle that more accurately though, it shouldn't be too hard since the initial byte indicates the length of the continuation, I was just a bit lazy on it.

@ds-sloth
Copy link
Author

It's good to hear that this shouldn't occur so long as the original utf-8 conversion went correctly. Feel free to close the issue.

Regarding the development time vs accuracy tradeoff, I totally understand. There are only so many hours in a day and as hobbyist developers we really have to allocate our time wisely. Just wanted to put the idea out there since I was reading through the source code and noticed there weren't checks that the indices stayed in bounds. Thanks!

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

No branches or pull requests

2 participants