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

Fix read off end of array due to bad pointer math in getworkingpath f… #4258

Closed
wants to merge 1 commit into from

Conversation

abramowitzK
Copy link

@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?

@abramowitzK abramowitzK marked this pull request as ready for review August 24, 2019 13:12
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Looks entirely correct!

@bagder
Copy link
Member

bagder commented Aug 24, 2019

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.

@bagder bagder closed this in 25f9621 Aug 24, 2019
@bagder
Copy link
Member

bagder commented Aug 24, 2019

Thanks!

@abramowitzK
Copy link
Author

No problem! Thanks for the library!

@lock lock bot locked as resolved and limited conversation to collaborators Nov 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants