You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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
I was running into issues with connections that were reaching CNTLM via SOCKS. First of all it would try to use a proxy server whereas according to the PAC file there would be a direct connection. It seems that specifying / as the URL was not what the rest of the code was expecting. I saw that other code was passing the hostname or IP address instead plus the port number and the hostname or IP address without a port number. Once I changed that,, connections were made correctly directly.
I also saw problems due to the incomplete data1 structure after the code was passing through prepare_http_connect, so I added the hostname and port number to it. This can result in a segfault later on as the data is expected to be there.
With these fixes in place CNTLM is not crashing anymore and handling SOCKS connections without problems. I guess the code is not perfect though, so I'd be happy to get any feedback that would help to improve things :)
Hello @jschwartzenberg this is a good catch. The code could be even simplified a bit, and I also suggest to use "hostname" instead of "thost_noport" (I know that it can contain an ip address rather than a real hostname) just to be consistent with other parts of the codebase. Also, a free is missing after strdup. I comment directly in the code diff.
Hi @fralken thank you so much for the feedback!! I was already testing with the frees that Sonar pointed me to, so I just pushed those as my tests were successful. I will work on the points you made and do another push!
That's great! I just added a comment on the main.c changes, because there is a case when hostname is freed without being allocated (some checks fail because of this).
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.Suggestion cannot be applied right now. Please check back later.
jschwartzenberg
force-pushed
the
fix-socks-proxying
branch
from
I was running into issues with connections that were reaching CNTLM via SOCKS. First of all it would try to use a proxy server whereas according to the PAC file there would be a direct connection. It seems that specifying
/as the URL was not what the rest of the code was expecting. I saw that other code was passing the hostname or IP address instead plus the port number and the hostname or IP address without a port number. Once I changed that,, connections were made correctly directly.I also saw problems due to the incomplete
data1structure after the code was passing throughprepare_http_connect, so I added the hostname and port number to it. This can result in a segfault later on as the data is expected to be there.With these fixes in place CNTLM is not crashing anymore and handling SOCKS connections without problems. I guess the code is not perfect though, so I'd be happy to get any feedback that would help to improve things :)