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

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 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.

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.
Copy link
Member

danielgustafsson left a comment

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?

@@ -327,18 +327,18 @@ CURLcode Curl_SOCKS4(const char *proxy_user,
* Make connection
*/
{
ssize_t packetsize = 9 +
size_t packetsize = 9 +

This comment has been minimized.

Copy link
@danielgustafsson

danielgustafsson Jul 5, 2020

Member

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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.