Control of 'addr' is now handed over to {platform_,}new_connection() and

sk_new() on invocation; these functions become responsible for (eventually)
freeing it. The caller must not do anything with 'addr' after it's been passed
in. (Ick.)

Why:
A SOCKS5 crash appears to have been caused by overzealous freeing of
a SockAddr (ssh.c:1.257 [r2492]), which for proxied connections is
squirreled away long-term (and this can't easily be avoided).

It would have been nice to make a copy of the SockAddr, in case the caller has
a use for it, but one of the implementations (uxnet.c) hides a "struct
addrinfo" in there, and we have no defined way to duplicate those. (None of the
current callers _do_ have a further use for the SockAddr.)

As far as I can tell, everything _except_ proxying only needs addr for the
duration of the call, so sk_addr_free()s immediately. If I'm mistaken, it
should at least be easier to find the offending free()...

[originally from svn r3383]
[r2492 == bdd6633970d9c53ce0ad97b460c8c684285bb15e]

diff --git a/mac/mtcpnet.c b/mac/mtcpnet.c
index 5415fbc6fde116a85c8b48473c538426dbccd74d..a8404b7a12b1202646694daef5f40f5923d5bd9f 100644 (file)
--- a/mac/mtcpnet.c
+++ b/mac/mtcpnet.c
@@ -517,6 +517,8 @@ Socket mactcp_new(SockAddr addr, int port, int privport, int oobinline,
        ret->next->prev = &ret->next;
     mactcp.socklist = ret;
 
+    sk_addr_free(addr); /* don't need this anymore */
+
     return (Socket)ret;
 }
 

diff --git a/mac/otnet.c b/mac/otnet.c
index 970a78e5bf5c7be6f04ce998ddd1fcddc94c4f6f..7881e02ad3edf69fc740c7a37ed8ea1b8ec64cf1 100644 (file)
--- a/mac/otnet.c
+++ b/mac/otnet.c
@@ -324,6 +324,8 @@ Socket ot_new(SockAddr addr, int port, int privport, int oobinline,
        ret->next->prev = &ret->next;
     ot.socklist = ret;
 
+    /* XXX: don't know whether we can sk_addr_free(addr); */
+
     return (Socket) ret;
 }
     

diff --git a/network.h b/network.h
index 46ec6bb746f987531248a9153e9380d61a60f83a..cb077fdb1f43b45ce186f2d2f3dba7d4d0747034 100644 (file)
--- a/network.h
+++ b/network.h
@@ -75,6 +75,8 @@ struct plug_function_table {
 };
 
 /* proxy indirection layer */
+/* NB, control of 'addr' is passed via new_connection, which takes
+ * responsibility for freeing it */
 Socket new_connection(SockAddr addr, char *hostname,
                      int port, int privport,
                      int oobinline, int nodelay, Plug plug,
@@ -85,6 +87,7 @@ SockAddr name_lookup(char *host, int port, char **canonicalname,
                     const Config *cfg);
 
 /* platform-dependent callback from new_connection() */
+/* (same caveat about addr as new_connection()) */
 Socket platform_new_connection(SockAddr addr, char *hostname,
                               int port, int privport,
                               int oobinline, int nodelay, Plug plug,
@@ -105,6 +108,8 @@ int sk_addrtype(SockAddr addr);
 void sk_addrcopy(SockAddr addr, char *buf);
 void sk_addr_free(SockAddr addr);
 
+/* NB, control of 'addr' is passed via sk_new, which takes responsibility
+ * for freeing it, as for new_connection() */
 Socket sk_new(SockAddr addr, int port, int privport, int oobinline,
              int nodelay, Plug p);
 

diff --git a/portfwd.c b/portfwd.c
index 00132ec6fa77578dea95e6ff2ede3cf41cb1218c..e61c05b43889e87aab67fcce2736ca3dcff06878 100644 (file)
--- a/portfwd.c
+++ b/portfwd.c
@@ -350,8 +350,10 @@ const char *pfd_newconnect(Socket *s, char *hostname, int port,
      * Try to find host.
      */
     addr = name_lookup(hostname, port, &dummy_realhost, cfg);
-    if ((err = sk_addr_error(addr)) != NULL)
+    if ((err = sk_addr_error(addr)) != NULL) {
+       sk_addr_free(addr);
        return err;
+    }
 
     /*
      * Open socket.
@@ -373,7 +375,6 @@ const char *pfd_newconnect(Socket *s, char *hostname, int port,
     }
 
     sk_set_private_ptr(*s, pr);
-    sk_addr_free(addr);
     return NULL;
 }
 

diff --git a/proxy.c b/proxy.c
index 4a2dd19c10724c6dab0bb7c3d5ea887c82283baf..169be0d1d139209f63265032607af8316c9ff6e7 100644 (file)
--- a/proxy.c
+++ b/proxy.c
@@ -90,6 +90,7 @@ static void sk_proxy_close (Socket s)
     Proxy_Socket ps = (Proxy_Socket) s;
 
     sk_close(ps->sub_socket);
+    sk_addr_free(ps->remote_addr);
     sfree(ps);
 }
 
@@ -391,7 +392,7 @@ Socket new_connection(SockAddr addr, char *hostname,
        ret->fn = &socket_fn_table;
        ret->cfg = *cfg;               /* STRUCTURE COPY */
        ret->plug = plug;
-       ret->remote_addr = addr;
+       ret->remote_addr = addr;       /* will need to be freed on close */
        ret->remote_port = port;
 
        ret->error = NULL;
@@ -443,8 +444,6 @@ Socket new_connection(SockAddr addr, char *hostname,
        if (sk_socket_error(ret->sub_socket) != NULL)
            return (Socket) ret;
 
-       sk_addr_free(proxy_addr);
-
        /* start the proxy negotiation process... */
        sk_set_frozen(ret->sub_socket, 0);
        ret->negotiate(ret, PROXY_CHANGE_NEW);

diff --git a/raw.c b/raw.c
index c493649316ae664c301ca1103aa86cd1031392db..4c9e5becd773111afcbbe3935a4cd83d80cfafe9 100644 (file)
--- a/raw.c
+++ b/raw.c
@@ -97,8 +97,10 @@ static const char *raw_init(void *frontend_handle, void **backend_handle,
        sfree(buf);
     }
     addr = name_lookup(host, port, realhost, cfg);
-    if ((err = sk_addr_error(addr)) != NULL)
+    if ((err = sk_addr_error(addr)) != NULL) {
+       sk_addr_free(addr);
        return err;
+    }
 
     if (port < 0)
        port = 23;                     /* default telnet port */
@@ -118,8 +120,6 @@ static const char *raw_init(void *frontend_handle, void **backend_handle,
     if ((err = sk_socket_error(raw->s)) != NULL)
        return err;
 
-    sk_addr_free(addr);
-
     return NULL;
 }
 

diff --git a/rlogin.c b/rlogin.c
index 455c2d9d2ba441c3fee1aa2b8477c0a9ff83c2a0..af3bd87633b4aeee1309f72c445bef4bfe2dc5a1 100644 (file)
--- a/rlogin.c
+++ b/rlogin.c
@@ -130,8 +130,10 @@ static const char *rlogin_init(void *frontend_handle, void **backend_handle,
        sfree(buf);
     }
     addr = name_lookup(host, port, realhost, cfg);
-    if ((err = sk_addr_error(addr)) != NULL)
+    if ((err = sk_addr_error(addr)) != NULL) {
+       sk_addr_free(addr);
        return err;
+    }
 
     if (port < 0)
        port = 513;                    /* default rlogin port */
@@ -151,8 +153,6 @@ static const char *rlogin_init(void *frontend_handle, void **backend_handle,
     if ((err = sk_socket_error(rlogin->s)) != NULL)
        return err;
 
-    sk_addr_free(addr);
-
     /*
      * Send local username, remote username, terminal/speed
      */

diff --git a/ssh.c b/ssh.c
index 8cb5a5645bd4325913bc28e7d6b30af668276f5e..23602f6d5438da1bd9ca41dcef9353eee404afe6 100644 (file)
--- a/ssh.c
+++ b/ssh.c
@@ -2169,7 +2169,6 @@ static const char *connect_to_host(Ssh ssh, char *host, int port,
     ssh->fn = &fn_table;
     ssh->s = new_connection(addr, *realhost, port,
                            0, 1, nodelay, (Plug) ssh, &ssh->cfg);
-    sk_addr_free(addr);
     if ((err = sk_socket_error(ssh->s)) != NULL) {
        ssh->s = NULL;
        return err;

diff --git a/telnet.c b/telnet.c
index f421cc58409aae12653ab11dbd9d6e0e4f828841..2edae2191e4aec0c7aeaa68e0e5df55d47cd47b1 100644 (file)
--- a/telnet.c
+++ b/telnet.c
@@ -710,8 +710,10 @@ static const char *telnet_init(void *frontend_handle, void **backend_handle,
        sfree(buf);
     }
     addr = name_lookup(host, port, realhost, &telnet->cfg);
-    if ((err = sk_addr_error(addr)) != NULL)
+    if ((err = sk_addr_error(addr)) != NULL) {
+       sk_addr_free(addr);
        return err;
+    }
 
     if (port < 0)
        port = 23;                     /* default telnet port */
@@ -731,8 +733,6 @@ static const char *telnet_init(void *frontend_handle, void **backend_handle,
     if ((err = sk_socket_error(telnet->s)) != NULL)
        return err;
 
-    sk_addr_free(addr);
-
     /*
      * Initialise option states.
      */

diff --git a/unix/uxnet.c b/unix/uxnet.c
index 7e5cf6f798b788d203458c8c6bef595adf191430..ce062d8a5acc38b34bd884d35cab725e9e7f3b9b 100644 (file)
--- a/unix/uxnet.c
+++ b/unix/uxnet.c
@@ -521,6 +521,8 @@ Socket sk_new(SockAddr addr, int port, int privport, int oobinline,
     uxsel_tell(ret);
     add234(sktree, ret);
 
+    sk_addr_free(addr);
+
     return (Socket) ret;
 }
 

diff --git a/unix/uxproxy.c b/unix/uxproxy.c
index cd256fdb66e1985ae18d0d57fea494afb5db4449..b3f75e813fabfa09fd6ff5bad6a2f984c20d6d88 100644 (file)
--- a/unix/uxproxy.c
+++ b/unix/uxproxy.c
@@ -299,5 +299,8 @@ Socket platform_new_connection(SockAddr addr, char *hostname,
 
     uxsel_set(ret->from_cmd, 1, localproxy_select_result);
 
+    /* We are responsible for this and don't need it any more */
+    sk_addr_free(addr);
+
     return (Socket) ret;
 }

diff --git a/winnet.c b/winnet.c
index 6052725b2d894c853ddcf7ceca9b56924cdc6fb0..c211f539da18573d92e54fa8ba8251eb743f4fc1 100644 (file)
--- a/winnet.c
+++ b/winnet.c
@@ -701,6 +701,9 @@ Socket sk_new(SockAddr addr, int port, int privport, int oobinline,
 
     add234(sktree, ret);
 
+    /* We're done with 'addr' now. */
+    sk_addr_free(addr);
+
     return (Socket) ret;
 }
 

diff --git a/x11fwd.c b/x11fwd.c
index 16944ee1bd9e1550c9b6dd609d6ff25e173f33fc..8b045f31b915b99faad179bf2b76473363c0223b 100644 (file)
--- a/x11fwd.c
+++ b/x11fwd.c
@@ -277,8 +277,10 @@ const char *x11_init(Socket * s, char *display, void *c, void *auth,
      * Try to find host.
      */
     addr = name_lookup(host, port, &dummy_realhost, cfg);
-    if ((err = sk_addr_error(addr)) != NULL)
+    if ((err = sk_addr_error(addr)) != NULL) {
+       sk_addr_free(addr);
        return err;
+    }
 
     /*
      * Open socket.
@@ -315,7 +317,6 @@ const char *x11_init(Socket * s, char *display, void *c, void *auth,
     }
 
     sk_set_private_ptr(*s, pr);
-    sk_addr_free(addr);
     return NULL;
 }