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

Jump to bottom

Update socks.c #5654

Closed
wants to merge 1 commit into from
Closed

Update socks.c #5654

wants to merge 1 commit into from

Conversation

ihsinme
Copy link
Contributor

@ihsinme ihsinme commented Jul 5, 2020

I propose to eliminate the risk of using the signed type (ssize_t) in the arithmetic of pointers, replacing it with an unsigned one (size_t). While in this context, the signed type (ssize_t) is used unnecessarily.

@ihsinme
Update socks.c
2a2d8bf 
I propose to eliminate the risk of using the signed type (ssize_t) in the arithmetic of pointers, replacing it with an unsigned one (size_t). While in this context, the signed type (ssize_t) is used unnecessarily.
danielgustafsson
danielgustafsson reviewed Jul 5, 2020
View reviewed changes
Copy link
Member

@danielgustafsson danielgustafsson left a comment

Choose a reason for hiding this comment

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

The outstanding member in struct connstate is of type ssize_t, so it makes sense to calculate using that type. Also, I'm not sure I see the the threat vector here, can you elaborate on why you think ssize_t is dangerous here?

lib/socks.c
@@ -327,18 +327,18 @@ CURLcode Curl_SOCKS4(const char *proxy_user,
* Make connection
*/
{
ssize_t packetsize = 9 +
size_t packetsize = 9 +
Copy link
Member

Choose a reason for hiding this comment

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

This is bogus indentation.

@ihsinme
Copy link
Contributor Author

ihsinme commented Jul 6, 2020

I see a threat in the following.
when the variables packetsize, hostnamelen or their sum increase beyond positive values, memory overflow to which socksreq packetsize points can occur. however, ongoing checks will not provide protection.

@bagder
Copy link
Member

bagder commented Jul 7, 2020

I don't think that's a "threat", but since the fix removes a typecast I still think its a good one.

@bagder bagder closed this in 60aa961 Jul 12, 2020
@bagder
Copy link
Member

bagder commented Jul 12, 2020

Thanks!

@ihsinme
Copy link
Contributor Author

ihsinme commented Jul 12, 2020

Thanks you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielgustafsson danielgustafsson danielgustafsson left review comments

Assignees
No one assigned
Labels
connecting & proxies
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ihsinme @bagder @danielgustafsson @jay