Fix read off end of array due to bad pointer math in getworkingpath f… #4258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…or SCP home directory case
|
Don't be alarmed by the red CI build(s), that's just due to flaky/bad environments and not because of any flaw in your PR. Ignore them. |
|
Thanks! |
|
No problem! Thanks for the library! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
@VictorSCushman and I came across this issue with help from Valgrind
The problem is reproducible by creating a basic application using the library to download a file from a url of this pattern: scp://user@host/~/* and running it through valgrind memcheck on Ubuntu 18.04. The actual error is an "invalid read of size 1"
We narrowed it down to this line below. Here's an explanation of our reasoning:
Given working_path as "/~/file.txt", working_path_len is calculated as 11 and real_path is a buffer allocated with a size of 12 (working_path_len + 1 for null terminator).
The memcpy below takes working_path + 3 as its source to strip the "/~/" leaving just "file.txt" which is of size 8 + 1 for null terminator. Therefore the length that should be memcopied should be 9 but below the length will be (4 + 11 - 3 = 12) and we will read off the end of working_path since our source pointer is actually working_path + 3 and our length is the full length of working_path including null terminator
I've tested this fix and it works fine for me and stops valgrind from complaining. Are there any problems that could be caused by this fix that my testing could have missed?