Fix: potential crash due to dropping client twice

Other trivial changes are included.
microyong · Apr 15, 2016 · 4f9ee0e · 4f9ee0e
1 parent 181b6d8
commit 4f9ee0e
Show file tree

Hide file tree

Showing 9 changed files with 64 additions and 99 deletions.
diff --git a/autoproxy.c b/autoproxy.c
@@ -159,7 +159,18 @@ static autoproxy_config * get_config(redsocks_client * client)
     }
 }
 
-#define get_autoproxy_client(client) (void*)(client + 1) + client->instance->relay_ss->payload_len;
+#define get_autoproxy_client(client) ((void*)(client + 1) + client->instance->relay_ss->payload_len)
+
+static void auto_release_recv_timer(autoproxy_client * aclient)
+{
+    // Cancel timer and release event object for timer
+    if (aclient->recv_timer_event)
+    {
+        event_del(aclient->recv_timer_event);
+        event_free(aclient->recv_timer_event);
+        aclient->recv_timer_event = NULL;
+    }
+}
 
 static void auto_client_init(redsocks_client *client)
 {
@@ -172,13 +183,7 @@ static void auto_client_init(redsocks_client *client)
 static void auto_client_fini(redsocks_client *client)
 {
     autoproxy_client * aclient = get_autoproxy_client(client);
-
-    if (aclient->recv_timer_event)
-    {
-        event_del(aclient->recv_timer_event);
-        event_free(aclient->recv_timer_event);
-        aclient->recv_timer_event = NULL;
-    }
+    auto_release_recv_timer(aclient);
 }
 
 static void on_connection_confirmed(redsocks_client *client)
@@ -204,13 +209,7 @@ static void auto_confirm_connection(redsocks_client * client)
         evbuffer_drain(bufferevent_get_input(client->client), aclient->data_sent);
         aclient->data_sent = 0;
     }
-    // Cancel timer and release event object for timer
-    if (aclient->recv_timer_event)
-    {
-        event_del(aclient->recv_timer_event);
-        event_free(aclient->recv_timer_event);
-        aclient->recv_timer_event = NULL;
-    }
+    auto_release_recv_timer(aclient);
     on_connection_confirmed(client);
 }
 
@@ -497,16 +496,9 @@ static int auto_retry(redsocks_client * client, int updcache)
                             inet_ntoa(client->destaddr.sin_addr));
         }
     }
-    // Release timer
-    if (aclient->recv_timer_event)
-    {
-        event_del(aclient->recv_timer_event);
-        event_free(aclient->recv_timer_event);
-        aclient->recv_timer_event = NULL;
-    }
 
+    auto_release_recv_timer(aclient);
     auto_drop_relay(client);
-
     // restore callbacks for ordinary client.
     bufferevent_setcb(client->client, NULL, NULL, redsocks_event_error, client);
     // enable reading to handle EOF from client
@@ -540,8 +532,7 @@ static int auto_retry_or_drop(redsocks_client * client)
     if (aclient->state == AUTOPROXY_NEW || aclient->state == AUTOPROXY_CONNECTED)
     {
         on_connection_blocked(client);  
-        auto_retry(client, 0);
-        return 0; 
+        return auto_retry(client, 0);
     }
     /* drop */
     return 1;
@@ -573,24 +564,18 @@ static void auto_relay_connected(struct bufferevent *buffev, void *_arg)
     The two peers will handle it. */
     bufferevent_set_timeouts(client->relay, NULL, NULL);
 
-    if (!redsocks_start_relay(client))
-    {
-        /* overwrite theread callback to my function */
-        bufferevent_setcb(client->client, direct_relay_clientreadcb,
-                                         direct_relay_clientwritecb,
-                                         auto_event_error,
-                                         client);
-        bufferevent_setcb(client->relay, direct_relay_relayreadcb,
-                                         direct_relay_relaywritecb,
-                                         auto_event_error,
-                                         client);
-    }
-    else
-    {
-        redsocks_log_error(client, LOG_DEBUG, "failed to start relay");
-        redsocks_drop_client(client);
+    if (redsocks_start_relay(client))
+        // redsocks_start_relay() drops client on failure
         return;
-    }
+    /* overwrite theread callback to my function */
+    bufferevent_setcb(client->client, direct_relay_clientreadcb,
+                                     direct_relay_clientwritecb,
+                                     auto_event_error,
+                                     client);
+    bufferevent_setcb(client->relay, direct_relay_relayreadcb,
+                                     direct_relay_relaywritecb,
+                                     auto_event_error,
+                                     client);
     // Write any data received from client side to relay.
     if (evbuffer_get_length(bufferevent_get_input(client->client)))
         direct_relay_relaywritecb(client->relay, client);
@@ -695,10 +680,6 @@ static int auto_connect_relay(redsocks_client *client)
     time_t * acc_time = NULL;
     time_t now = redsocks_time(NULL);   
 
-    /* use default timeout if timeout is not configured */
-    if (tv.tv_sec == 0)
-        tv.tv_sec = DEFAULT_CONNECT_TIMEOUT; 
-
     if (aclient->state == AUTOPROXY_NEW)
     {
         acc_time = cache_get_addr_time(&client->destaddr);

diff --git a/direct.c b/direct.c
@@ -51,11 +51,8 @@ static void direct_write_cb(struct bufferevent *buffev, void *_arg)
     {
         client->state = 1;
         if (redsocks_start_relay(client))
-        {
-            // Failed to start relay. Drop connection.
-            redsocks_drop_client(client);
+            // Failed to start relay. Connection is dropped.
             return;
-        }
         // Write any data received from client to relay
         if (evbuffer_get_length(bufferevent_get_input(client->client)))
             client->instance->relay_ss->writecb(buffev, client);

diff --git a/redsocks.c b/redsocks.c
@@ -207,6 +207,8 @@ static int redsocks_onexit(parser_section *section)
     if (err)
         parser_error(section->context, "%s", err);
 
+    if (instance->config.timeout == 0)
+        instance->config.timeout = DEFAULT_CONNECT_TIMEOUT;
     return err ? -1 : 0;
 }
 
@@ -638,9 +640,6 @@ int redsocks_connect_relay(redsocks_client *client)
     struct timeval tv;
     tv.tv_sec = client->instance->config.timeout;
     tv.tv_usec = 0;
-    if (tv.tv_sec == 0)
-        tv.tv_sec = DEFAULT_CONNECT_TIMEOUT;
-
 
     client->relay = red_connect_relay2(&client->instance->config.relayaddr,
                                       NULL, 
@@ -713,7 +712,7 @@ static void redsocks_accept_client(int fd, short what, void *_arg)
         if (errno == ENFILE || errno == EMFILE || errno == ENOBUFS || errno == ENOMEM) {
             self->accept_backoff_ms = (self->accept_backoff_ms << 1) + 1;
             clamp_value(self->accept_backoff_ms, self->config.min_backoff_ms, self->config.max_backoff_ms);
-            int delay = (random() % self->accept_backoff_ms) + 1;
+            int delay = (red_randui32() % self->accept_backoff_ms) + 1;
             log_errno(LOG_WARNING, "accept: out of file descriptors, backing off for %u ms", delay);
             struct timeval tvdelay = { delay / 1000, (delay % 1000) * 1000 };
             if (tracked_event_del(&self->listener) != 0)
@@ -1015,7 +1014,7 @@ static void redsocks_fini_instance(redsocks_instance *instance) {
         if (timerisset(&instance->listener.inserted))
             if (tracked_event_del(&instance->listener) != 0)
                 log_errno(LOG_WARNING, "event_del");
-        redsocks_close(EVENT_FD(&instance->listener.ev));
+        redsocks_close(event_get_fd(&instance->listener.ev));
         memset(&instance->listener, 0, sizeof(instance->listener));
     }
 

diff --git a/redudp.c b/redudp.c
@@ -282,7 +282,7 @@ void redudp_fwd_pkt_to_sender(redudp_client *client, void *buf, size_t len,
     // When working with TPROXY, we have to get sender FD from tree on
     // receipt of each packet from relay.
     fd = do_tproxy(client->instance) ? bound_udp4_get(srcaddr)
-                                     : EVENT_FD(&client->instance->listener);
+                                     : event_get_fd(&client->instance->listener);
     if (fd == -1) {
         redudp_log_error(client, LOG_WARNING, "bound_udp4_get failure");
         return;
@@ -392,7 +392,7 @@ static void redudp_pkt_from_client(int fd, short what, void *_arg)
 
     pdestaddr = do_tproxy(self) ? &destaddr : NULL;
 
-    assert(fd == EVENT_FD(&self->listener));
+    assert(fd == event_get_fd(&self->listener));
     // destaddr will be filled with true destination if it is available
     pktlen = red_recv_udp_pkt(fd, recv_buff, sizeof(recv_buff), &clientaddr, pdestaddr);
     if (pktlen == -1)
@@ -628,7 +628,7 @@ static void redudp_fini_instance(redudp_instance *instance)
     if (event_initialized(&instance->listener)) {
         if (event_del(&instance->listener) != 0)
             log_errno(LOG_WARNING, "event_del");
-        close(EVENT_FD(&instance->listener));
+        close(event_get_fd(&instance->listener));
         memset(&instance->listener, 0, sizeof(instance->listener));
     }
 

diff --git a/shadowsocks-udp.c b/shadowsocks-udp.c
@@ -67,7 +67,7 @@ static void ss_client_fini(redudp_client *client)
 {
     ss_client *ssclient = (void*)(client + 1);
     if (event_initialized(&ssclient->udprelay)) {
-        close(EVENT_FD(&ssclient->udprelay));
+        close(event_get_fd(&ssclient->udprelay));
         if (event_del(&ssclient->udprelay) == -1)
             redudp_log_errno(client, LOG_ERR, "event_del");
     }
@@ -121,7 +121,7 @@ static void ss_forward_pkt(redudp_client *client, struct sockaddr * destaddr, vo
     io[0].iov_base = ss->buff;
     io[0].iov_len = fwdlen;
 
-    outgoing = sendmsg(EVENT_FD(&ssclient->udprelay), &msg, 0);
+    outgoing = sendmsg(event_get_fd(&ssclient->udprelay), &msg, 0);
     if (outgoing == -1) {
         redudp_log_errno(client, LOG_DEBUG, "sendmsg: Can't forward packet, dropping it");
         return;
@@ -143,7 +143,7 @@ static void ss_pkt_from_server(int fd, short what, void *_arg)
     struct sockaddr_in udprelayaddr;
     int rc;
 
-    assert(fd == EVENT_FD(&ssclient->udprelay));
+    assert(fd == event_get_fd(&ssclient->udprelay));
 
     pktlen = red_recv_udp_pkt(fd, ss->buff, SHARED_BUFF_SIZE, &udprelayaddr, NULL);
     if (pktlen == -1)

diff --git a/shadowsocks.c b/shadowsocks.c
@@ -25,8 +25,6 @@
 #include "encrypt.h"
 #include "shadowsocks.h"
 
-#define INITIAL_BUFFER_SIZE 8192
-
 typedef enum ss_state_t {
     ss_new,
     ss_connected,
@@ -292,24 +290,18 @@ static void ss_relay_connected(struct bufferevent *buffev, void *_arg)
     The two peers will handle it. */
     bufferevent_set_timeouts(client->relay, NULL, NULL);
 
-    if (!redsocks_start_relay(client))
-    {
-        /* overwrite theread callback to my function */
-        bufferevent_setcb(client->client, ss_client_readcb,
-                                         ss_client_writecb,
-                                         redsocks_event_error,
-                                         client);
-        bufferevent_setcb(client->relay, ss_relay_readcb,
-                                         ss_relay_writecb,
-                                         redsocks_event_error,
-                                         client);
-    }
-    else
-    {
-        redsocks_log_error(client, LOG_DEBUG, "failed to start relay");
-        redsocks_drop_client(client);
+    if (redsocks_start_relay(client))
+        // redsocks_start_relay() drops client on failure
         return;
-    }
+    /* overwrite theread callback to my function */
+    bufferevent_setcb(client->client, ss_client_readcb,
+                                     ss_client_writecb,
+                                     redsocks_event_error,
+                                     client);
+    bufferevent_setcb(client->relay, ss_relay_readcb,
+                                     ss_relay_writecb,
+                                     redsocks_event_error,
+                                     client);
 
     /* build and send header */
     // TODO: Better implementation and IPv6 Support
@@ -335,10 +327,6 @@ static int ss_connect_relay(redsocks_client *client)
 
     tv.tv_sec = client->instance->config.timeout;
     tv.tv_usec = 0;
-    /* use default timeout if timeout is not configured */
-    if (tv.tv_sec == 0)
-        tv.tv_sec = DEFAULT_CONNECT_TIMEOUT; 
-
     client->relay = red_connect_relay2(&client->instance->config.relayaddr,
                     NULL, ss_relay_connected, redsocks_event_error, client, 
                     &tv);

diff --git a/socks5-udp.c b/socks5-udp.c
@@ -87,7 +87,7 @@ static void socks5_client_fini(redudp_client *client)
 	int fd;
 
 	if (event_initialized(&socks5client->udprelay)) {
-		fd = EVENT_FD(&socks5client->udprelay);
+		fd = event_get_fd(&socks5client->udprelay);
 		if (event_del(&socks5client->udprelay) == -1)
 			redudp_log_errno(client, LOG_ERR, "event_del");
 		close(fd);
@@ -127,7 +127,7 @@ static void socks5_forward_pkt(redudp_client *client, struct sockaddr *destaddr,
 	io[1].iov_base = buf;
 	io[1].iov_len = pktlen;
 
-	outgoing = sendmsg(EVENT_FD(&socks5client->udprelay), &msg, 0);
+	outgoing = sendmsg(event_get_fd(&socks5client->udprelay), &msg, 0);
 	if (outgoing == -1) {
 		redudp_log_errno(client, LOG_WARNING, "sendmsg: Can't forward packet, dropping it");
 		return;
@@ -149,7 +149,7 @@ static void socks5_pkt_from_socks(int fd, short what, void *_arg)
 	ssize_t pktlen, fwdlen;
 	struct sockaddr_in udprelayaddr;
 
-	assert(fd == EVENT_FD(&socks5client->udprelay));
+	assert(fd == event_get_fd(&socks5client->udprelay));
 
 	pktlen = red_recv_udp_pkt(fd, pkt.buf, sizeof(pkt.buf), &udprelayaddr, NULL);
 	if (pktlen == -1)

diff --git a/tcpdns.c b/tcpdns.c
@@ -126,7 +126,7 @@ static void tcpdns_readcb(struct bufferevent *from, void *_arg)
                 case DNS_RC_FORMERR:
                 case DNS_RC_NXDOMAIN:
                     {
-                        int fd = EVENT_FD(&req->instance->listener);
+                        int fd = event_get_fd(&req->instance->listener);
                         if (sendto(fd, &buff.raw[2], read_size - 2, 0,
                                 (struct sockaddr*)&req->client_addr,
                                 sizeof(req->client_addr)) != read_size - 2) {
@@ -276,7 +276,7 @@ static void tcpdns_pkt_from_client(int fd, short what, void *_arg)
     struct sockaddr_in * destaddr;
     ssize_t pktlen;
 
-    assert(fd == EVENT_FD(&self->listener));
+    assert(fd == event_get_fd(&self->listener));
     /* allocate and initialize request structure */
     req = (dns_request *)calloc(sizeof(dns_request), 1);
     if (!req)
@@ -493,7 +493,7 @@ static void tcpdns_fini_instance(tcpdns_instance *instance)
     if (event_initialized(&instance->listener)) {
         if (event_del(&instance->listener) != 0)
             log_errno(LOG_WARNING, "event_del");
-        if (close(EVENT_FD(&instance->listener)) != 0)
+        if (close(event_get_fd(&instance->listener)) != 0)
             log_errno(LOG_WARNING, "close");
     }