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

Non-null terminated C string in openosc_get_process_name #5

Closed
ariel-miculas opened this issue Nov 20, 2023 · 6 comments
Closed

Non-null terminated C string in openosc_get_process_name #5

ariel-miculas opened this issue Nov 20, 2023 · 6 comments

Comments

@ariel-miculas
Copy link

ariel-miculas commented Nov 20, 2023

https://github.com/cisco/OpenOSC/blob/master/src/openosc_support.c#L58-L62
If the last character from /proc/%d/cmdline is not '\n', the string will not be NUL terminated.
Found by @hallyn

@yonhan3
Copy link
Contributor

yonhan3 commented Dec 7, 2023

Thanks for reporting this issue.
Yes, this function will return non-NULL terminated string in some cases.
However, since this function is a static function, which is only used by another function in this same file at line 104. And the other function does make it NULL terminated at line 109. So the impact is limited.
But we will try to improve this function in next release of OpenOSC.

BTW, the cmdline file is special, because it contains multiple NULL-terminated strings. We just need the first NULL terminated string.

 hexdump -C /proc/self/cmdline
00000000  68 65 78 64 75 6d 70 00  2d 43 00 2f 70 72 6f 63  |hexdump.-C./proc|
00000010  2f 73 65 6c 66 2f 63 6d  64 6c 69 6e 65 00        |/self/cmdline.|
0000001e

@ariel-miculas
Copy link
Author

Yes, but if the file path is too large, then the string will not be NUL terminated. And you have a call to strlen which may end up in a segmentation fault if you're unlucky enough.

@yonhan3
Copy link
Contributor

yonhan3 commented Dec 8, 2023

Yes, agree. I will fix the get_process_name function to always return NULL-terminated string.

@yonhan3
Copy link
Contributor

yonhan3 commented Jan 24, 2024

This issue has been fixed by commit #6
The new v1.0.7 version has the fix.

@yonhan3 yonhan3 closed this as completed Jan 24, 2024
@hallyn
Copy link

hallyn commented Jan 24, 2024

The 'commit #6' above is not right: since you're not using pull requests, github just links it to issue #6, which does not exist.

The correct commit is, I assume:

d1bd20c

@yonhan3
Copy link
Contributor

yonhan3 commented Jan 24, 2024

Yes, you are right. Thanks for pointing this out. :-)

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

3 participants