Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Use snprintf instead of sprintf #4

Closed
wants to merge 2 commits into from
Closed

Conversation

jclehner
Copy link
Contributor

Use snprintf instead of sprintf to avoid potential (security) issues when encountering a ridiculously long CLASSPATH.

@rovo89
Copy link
Owner

rovo89 commented Dec 13, 2013

Thanks. That would avoid overwriting memory (it would reserve one byte for the terminating character, right?). But there would likely be a crash in that (theoretical) case where anyone sets such a long CLASSPATH because anything after 4095 characters is cut off. So I think such a check would require checking whether the current classpath plus separator and the jar path fit into the buffer and don't even attempt to add Xposed in that case. I think this will never happen anyway, but if we touch this, I would like to make sure that it's a complete fix.

@jclehner
Copy link
Contributor Author

Okay this patch lets addXposedToClasspath return false if the string would be longer than 4096 bytes. As to your question regarding snprintf: the buffer size includes the \0 byte.

@rovo89
Copy link
Owner

rovo89 commented Dec 13, 2013

Ok, that looks like an elegant solution.

@rovo89
Copy link
Owner

rovo89 commented Dec 13, 2013

I need to test it myself before merging it though ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants