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

--connect-to incorrect destination port 0 #1148

Closed
jay opened this Issue Nov 29, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@jay
Member

jay commented Nov 29, 2016

I did this

> curld -v --connect-to foo:123:bar:456 https://curl.haxx.se
* STATE: INIT => CONNECT handle 0x7b1ae0; line 1413 (connection #-5000)
* Rebuilt URL to: https://curl.haxx.se/
* Connecting to port: 0
* Added connection 0. The cache now contains 1 members
* STATE: CONNECT => WAITRESOLVE handle 0x7b1ae0; line 1450 (connection #0)
*   Trying 80.67.6.50...
* TCP_NODELAY set
* Immediate connect fail for 80.67.6.50: Address not available
* Closing connection 0
* The cache now contains 0 members
* Expire cleared
curl: (7) Couldn't connect to server

I expected the following

If the hostname and port combination of the requested URL is not in the connect-to entry list then it shouldn't change the hostname or the port.

analysis

connect-to lists have each entry in the format HOST:PORT:CONNECT-TO-HOST:CONNECT-TO-PORT.

parse_connect_to_slist is called by create_conn to parse the connect-to list and if a HOST:PORT match is found assign CONNECT-TO-HOST:CONNECT-TO-PORT to the connection. A function-local port variable is initialized to 0, and may or may not be changed by the parser to -1 on no match. Later when checking for a match it treats port 0 as a match. That does not make sense to me, who is using port 0? It looks like allowing that port might have been done purposely since no-match is signaled by -1.

In more detail parse_connect_to_slist calls parse_connect_to_string to parse HOST:PORT and if a match is found that function calls parse_connect_to_host_port to get the CONNECT-TO-HOST:CONNECT-TO-PORT. If the connect-to host and port are empty parse_connect_to_host_port will set the outvar port -1 and outvar host NULL as seen here to indicate no match.

The problem is if there is no match in the HOST:PORT then the outvar host and port (ie the result of parsing CONNECT-TO-HOST:CONNECT-TO-PORT) never receive the values. With host it starts as NULL so there's never a problem but with port it starts as 0 so that's a problem. A possible fix could be we set the host and port outvars first thing in parse_connect_to_string, much in the same way they're already set in parse_connect_to_host_port

diff --git a/lib/url.c b/lib/url.c
index 9ee1e6c..e0ea980 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -5677,6 +5677,9 @@ static CURLcode parse_connect_to_string(struct Curl_easy *data,
   int host_match = FALSE;
   int port_match = FALSE;
 
+  *host_result = NULL;
+  *port_result = -1;
+
   if(*ptr == ':') {
     /* an empty hostname always matches */
     host_match = TRUE;

Also maybe not initialize port to 0 if it's a valid value? (and again why is this)

curl/libcurl version

master curl-7_51_0-112-g19613fb 2016-11-28

curl 7.52.0-DEV (i386-pc-win32) libcurl/7.52.0-DEV OpenSSL/1.0.2j nghttp2/1.16.1
Protocols: dict file ftp ftps gopher http https imap imaps ldap pop3 pop3s rtsp
smb smbs smtp smtps telnet tftp
Features: AsynchDNS Debug Largefile NTLM SSL HTTP2 HTTPS-proxy

operating system

Windows 7 Enterprise x64

/cc @mkauf

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 29, 2016

Member

Port 0 is actually a fully legal and working port in TCP and UDP and this is a fine chance for me to link to my own blog post on the subject. The problem with port 0 is primarily for APIs that use zero to mean something else than a port number. I don't think we need to add to those problems so we should instead use a value outside of the 16 bit range to mark it as a non-existing port. -1 seems reasonable to me!

Member

bagder commented Nov 29, 2016

Port 0 is actually a fully legal and working port in TCP and UDP and this is a fine chance for me to link to my own blog post on the subject. The problem with port 0 is primarily for APIs that use zero to mean something else than a port number. I don't think we need to add to those problems so we should instead use a value outside of the 16 bit range to mark it as a non-existing port. -1 seems reasonable to me!

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 29, 2016

Member

Port 0 is actually a fully legal and working port in TCP and UDP

Well sometimes then. I get no outgoing packets in Windows when I try to connect to an ip with port 0. In Linux I get conn refused though. Interesting post.

Member

jay commented Nov 29, 2016

Port 0 is actually a fully legal and working port in TCP and UDP

Well sometimes then. I get no outgoing packets in Windows when I try to connect to an ip with port 0. In Linux I get conn refused though. Interesting post.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 29, 2016

Member

I think you can proceed and merge your suggested change, including initializing port to -1 in parse_connect_to_slist.

Member

bagder commented Nov 29, 2016

I think you can proceed and merge your suggested change, including initializing port to -1 in parse_connect_to_slist.

@mkauf

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Nov 29, 2016

Contributor

Thank you for the bug report! I think I found an additional small bug in the "while" condition. The loop should stop if a "connect to port" has been found.

Proposed bugfix:

diff --git a/lib/url.c b/lib/url.c
index 9ee1e6c..dd3f62d 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -5677,6 +5677,9 @@
   int host_match = FALSE;
   int port_match = FALSE;
 
+  *host_result = NULL;
+  *port_result = -1;
+
   if(*ptr == ':') {
     /* an empty hostname always matches */
     host_match = TRUE;
@@ -5739,9 +5742,9 @@
 {
   CURLcode result = CURLE_OK;
   char *host = NULL;
-  int port = 0;
+  int port = -1;
 
-  while(conn_to_host && !host) {
+  while(conn_to_host && !host && port == -1) {
     result = parse_connect_to_string(data, conn, conn_to_host->data,
                                      &host, &port);
     if(result)
@@ -5760,7 +5763,7 @@
     else {
       /* no "connect to host" */
       conn->bits.conn_to_host = FALSE;
-      free(host);
+      Curl_safefree(host);
     }
 
     if(port >= 0) {
@@ -5771,6 +5774,7 @@
     else {
       /* no "connect to port" */
       conn->bits.conn_to_port = FALSE;
+      port = -1;
     }
 
     conn_to_host = conn_to_host->next;
Contributor

mkauf commented Nov 29, 2016

Thank you for the bug report! I think I found an additional small bug in the "while" condition. The loop should stop if a "connect to port" has been found.

Proposed bugfix:

diff --git a/lib/url.c b/lib/url.c
index 9ee1e6c..dd3f62d 100644
--- a/lib/url.c
+++ b/lib/url.c
@@ -5677,6 +5677,9 @@
   int host_match = FALSE;
   int port_match = FALSE;
 
+  *host_result = NULL;
+  *port_result = -1;
+
   if(*ptr == ':') {
     /* an empty hostname always matches */
     host_match = TRUE;
@@ -5739,9 +5742,9 @@
 {
   CURLcode result = CURLE_OK;
   char *host = NULL;
-  int port = 0;
+  int port = -1;
 
-  while(conn_to_host && !host) {
+  while(conn_to_host && !host && port == -1) {
     result = parse_connect_to_string(data, conn, conn_to_host->data,
                                      &host, &port);
     if(result)
@@ -5760,7 +5763,7 @@
     else {
       /* no "connect to host" */
       conn->bits.conn_to_host = FALSE;
-      free(host);
+      Curl_safefree(host);
     }
 
     if(port >= 0) {
@@ -5771,6 +5774,7 @@
     else {
       /* no "connect to port" */
       conn->bits.conn_to_port = FALSE;
+      port = -1;
     }
 
     conn_to_host = conn_to_host->next;
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Nov 29, 2016

Member

👍 looks good to me!

Member

bagder commented Nov 29, 2016

👍 looks good to me!

@jay

This comment has been minimized.

Show comment
Hide comment
@jay

jay Nov 29, 2016

Member

👍 me too, go for it

Member

jay commented Nov 29, 2016

👍 me too, go for it

@mkauf mkauf closed this in b34ea05 Nov 30, 2016

@mkauf

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Nov 30, 2016

Contributor

Fixed, thanks!

Contributor

mkauf commented Nov 30, 2016

Fixed, thanks!

@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018

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