Merge lp:~schuster/mysql-proxy/remove_unix_socket into lp:mysql-proxy

[Michael Schuster]

[Jan Kneschke (jan-kneschke)]

Am 19.04.2010 um 08:48 schrieb Michael Schuster:

> Michael Schuster has proposed merging lp:~schuster/mysql-proxy/remove_unix_socket into lp:mysql-proxy.
>
> Requested reviews:
> MySQL Proxy Developers (mysql-proxy-developers)
>
> --
> https://code.launchpad.net/~schuster/mysql-proxy/remove_unix_socket/+merge/23652
> Your team MySQL Proxy Developers is subscribed to branch lp:mysql-proxy.
> === modified file 'src/network-address.c'
> --- src/network-address.c 2010-04-07 12:44:32 +0000
> +++ src/network-address.c 2010-04-19 06:48:19 +0000
> @@ -45,6 +45,8 @@
> #include <string.h>
> #include <errno.h>
> #include <fcntl.h>
> +#include <glib.h>
> +#include <glib/gstdio.h>
>
> #include "network-address.h"
> #include "glib-ext.h"
> @@ -63,10 +65,33 @@
> }
>
> void network_address_free(network_address *addr) {
> +
> if (!addr) return;
>
> +#ifndef WIN32
> + /*
> + * if the name we're freeing starts with a '/', we're
> + * looking at a unix socket which needs to be removed
> + */
> + if (addr->fail_errno == 0 && addr->name != NULL &&
> + addr->name->str != NULL) {
> + gchar *name;
> + int ret;
> +
> + name = addr->name->str;
> + if (name[0] == '/' && g_access(name, 0) == 0) {
> + ret = g_remove(name);

Only try to remove it if we can to hide the error-msg (aka noise) for it ?

> +
> + if (ret == 0)
> + g_debug("%s removing socket %s successful",
> + G_STRLOC, name);
> + else
> + g_debug("%s removing socket %s failed: %s (%d)",
> + G_STRLOC, name, strerror(errno));

... in that case the g_debug() is perhaps better a g_critical() ?

> + }
> + }
> +#endif /* WIN32 */
> +
> g_string_free(addr->name, TRUE);
> -
> g_free(addr);
> }
>
>
> === modified file 'src/network-address.h'
> --- src/network-address.h 2010-04-06 14:26:51 +0000
> +++ src/network-address.h 2010-04-19 06:48:19 +0000
> @@ -63,8 +63,8 @@
> } addr;
>
> GString *name;
> -
> network_socklen_t len;
> + int fail_errno; /* != 0: don't remove ... I guess */
> } network_address;
>
> NETWORK_API network_address *network_address_new(void);
>
> === modified file 'src/network-socket.c'
> --- src/network-socket.c 2010-04-06 14:26:51 +0000
> +++ src/network-socket.c 2010-04-19 06:48:19 +0000
> @@ -397,6 +397,7 @@
> G_STRLOC,
> con->dst->name->str,
> g_strerror(errno), errno);
> + con->dst->fail_errno = errno; /* if a unix socket, this tells us "don't remove" */
> return NETWORK_SOCKET_ERROR;
> }
>
>

[Michael Schuster (schuster)]

On 04/19/10 09:39, Jan Kneschke wrote:
> Am 19.04.2010 um 08:48 schrieb Michael Schuster:
>
>> Michael Schuster has proposed merging lp:~schuster/mysql-proxy/remove_unix_socket into lp:mysql-proxy.
>>
>> Requested reviews:
>> MySQL Proxy Developers (mysql-proxy-developers)
>>
>> --
>> https://code.launchpad.net/~schuster/mysql-proxy/remove_unix_socket/+merge/23652
>> Your team MySQL Proxy Developers is subscribed to branch lp:mysql-proxy.
>> === modified file 'src/network-address.c'
>> --- src/network-address.c 2010-04-07 12:44:32 +0000
>> +++ src/network-address.c 2010-04-19 06:48:19 +0000
>> @@ -45,6 +45,8 @@
>> #include <string.h>
>> #include <errno.h>
>> #include <fcntl.h>
>> +#include <glib.h>
>> +#include <glib/gstdio.h>
>>
>> #include "network-address.h"
>> #include "glib-ext.h"
>> @@ -63,10 +65,33 @@
>> }
>>
>> void network_address_free(network_address *addr) {
>> +
>> if (!addr) return;
>>
>> +#ifndef WIN32
>> + /*
>> + * if the name we're freeing starts with a '/', we're
>> + * looking at a unix socket which needs to be removed
>> + */
>> + if (addr->fail_errno == 0 && addr->name != NULL &&
>> + addr->name->str != NULL) {
>> + gchar *name;
>> + int ret;
>> +
>> + name = addr->name->str;
>> + if (name[0] == '/' && g_access(name, 0) == 0) {
>> + ret = g_remove(name);
>
> Only try to remove it if we can to hide the error-msg (aka noise) for it ?

sorry, don't understand the question :-(

>> +
>> + if (ret == 0)
>> + g_debug("%s removing socket %s successful",
>> + G_STRLOC, name);
>> + else
>> + g_debug("%s removing socket %s failed: %s (%d)",
>> + G_STRLOC, name, strerror(errno));
>
> ... in that case the g_debug() is perhaps better a g_critical() ?

ok.

Michael

[lp:~schuster/mysql-proxy/remove_unix_socket]

1049. By Michael Schuster on 2010-04-23

change warning level for failure to remove socket

[Jan Kneschke (jan-kneschke)]

Am 19.04.2010 um 10:45 schrieb Michael Schuster:

> On 04/19/10 09:39, Jan Kneschke wrote:
>> Am 19.04.2010 um 08:48 schrieb Michael Schuster:
>>
>>> Michael Schuster has proposed merging lp:~schuster/mysql-proxy/remove_unix_socket into lp:mysql-proxy.
>>>
>>> Requested reviews:
>>> MySQL Proxy Developers (mysql-proxy-developers)
>>>
>>> --
>>> https://code.launchpad.net/~schuster/mysql-proxy/remove_unix_socket/+merge/23652
>>> Your team MySQL Proxy Developers is subscribed to branch lp:mysql-proxy.
>>> === modified file 'src/network-address.c'
>>> --- src/network-address.c 2010-04-07 12:44:32 +0000
>>> +++ src/network-address.c 2010-04-19 06:48:19 +0000
>>> @@ -45,6 +45,8 @@
>>> #include <string.h>
>>> #include <errno.h>
>>> #include <fcntl.h>
>>> +#include <glib.h>
>>> +#include <glib/gstdio.h>
>>>
>>> #include "network-address.h"
>>> #include "glib-ext.h"
>>> @@ -63,10 +65,33 @@
>>> }
>>>
>>> void network_address_free(network_address *addr) {
>>> +
>>> if (!addr) return;
>>>
>>> +#ifndef WIN32
>>> + /*
>>> + * if the name we're freeing starts with a '/', we're
>>> + * looking at a unix socket which needs to be removed
>>> + */
>>> + if (addr->fail_errno == 0 && addr->name != NULL &&
>>> + addr->name->str != NULL) {
>>> + gchar *name;
>>> + int ret;
>>> +
>>> + name = addr->name->str;
>>> + if (name[0] == '/' && g_access(name, 0) == 0) {
>>> + ret = g_remove(name);
>>
>> Only try to remove it if we can to hide the error-msg (aka noise) for it ?
>
> sorry, don't understand the question :-(

I tried to figure out the reason for the g_access() and assumed it is to check if the g_remove() could succeed without a permission problem.

Problem with access() is that it creates a race-condition before the the permission check and the g_remove(). Instead just unlink() it and check for the 'errno != EPERM' to see if we can't delete the socket as we drop the privileges.

>>> +
>>> + if (ret == 0)
>>> + g_debug("%s removing socket %s successful",
>>> + G_STRLOC, name);
>>> + else
>>> + g_debug("%s removing socket %s failed: %s (%d)",
>>> + G_STRLOC, name, strerror(errno));
>>
>> ... in that case the g_debug() is perhaps better a g_critical() ?
>
> ok.

The whole patch does unlink() any file that is set as address. In the case of a network_socket_new() + network_address_set_address() + network_socket_connect() + network_socket_free() the socket-file should not be unlinked as it we didn't listen() on it.

Instead of ->fail_errno use a gboolean to track if you want to unlink the socket file on free and only set it to true in the case of a successful network_socket_bind().

> Michael
> --
> https://code.launchpad.net/~schuster/mysql-proxy/remove_unix_socket/+merge/23652
> Your team MySQL Proxy Developers is subscribed to branch lp:mysql-proxy.

[Jan Kneschke (jan-kneschke)]

A test-case to verify that we don't unlink() socket-files we havn't bound

[lp:~schuster/mysql-proxy/remove_unix_socket]

1050. By Michael Schuster on 2010-04-29

remove unix socket at shutdown: remove race window, make logic for "remove the once we created only" better

1051. By Michael Schuster on 2010-05-03

add check for local socket removal to "is local" testcase

1052. By Michael Schuster on 2010-05-03

make sure socket is removed if assertion fails: add sig handler for SIGARBT

[Jan Kneschke (jan-kneschke)]

* remove the commented lines:

  /* printf("debug: socket name is %s (addr: %p)\n", pp->sockname, pp); */
  /* g_test_add_func("/core/network_socket_is_local_unix", t_network_socket_is_local_unix);*/

* stay with the existing coding style (no line-break after function declarations)
* The signal-handler for SIGABRT is installed for _all_ tests of this test-suite. Instead it should be added in the setup and turned back into SIG_DFL in teardown.
* void t_network_socket_is_local_unix() is about the ..._is_local() function. Comments are missing to describe that it also tests for 'socket is removed after successful bind() only'. It would have been better to have seperate test that only tests that.

[Jan Kneschke (jan-kneschke)]

g_debug("%s removing socket %s successful", ... is missing a colon after the first %s

[Jan Kneschke (jan-kneschke)]

review: Needs Fixing

[Michael Schuster (schuster)]

Jan,

thx for review, comments in-line.

On 04.05.10 13:50, Jan Kneschke wrote:
> * remove the commented lines:
>
> /* printf("debug: socket name is %s (addr: %p)\n", pp->sockname, pp); */
> /* g_test_add_func("/core/network_socket_is_local_unix", t_network_socket_is_local_unix);*/

done

> * stay with the existing coding style (no line-break after function declarations)

ack, done.

> * The signal-handler for SIGABRT is installed for _all_ tests of this test-suite. Instead it should be added in the setup and turned back into SIG_DFL in teardown.

done

> * void t_network_socket_is_local_unix() is about the ..._is_local() function. Comments are missing to describe that it also tests for 'socket is removed after successful bind() only'. It would have been better to have seperate test that only tests that.

ack

> g_debug("%s removing socket %s successful", ... is missing a colon after the first %s

this is now gone.

Michael
--
Michael Schuster http://blogs.sun.com/recursion
Recursion, n.: see 'Recursion'

[lp:~schuster/mysql-proxy/remove_unix_socket]

1053. By Michael Schuster on 2010-05-10

fix diversion

[Jan Kneschke (jan-kneschke)]

approved. will merge it to trunk

review: Approve

[Jan Kneschke (jan-kneschke)]

65 === modified file 'src/network-socket.c'
66 --- src/network-socket.c 2010-05-03 14:26:32 +0000
67 +++ src/network-socket.c 2010-05-10 11:52:26 +0000
...

In static network_socket_retval_t network_socket_write_send(network_socket *con, int send_chunks) is

76 @@ -660,6 +661,7 @@
77 }
78 }
79
80 + con->dst->can_unlink_socket = TRUE;
81 return NETWORK_SOCKET_SUCCESS;
82 }

which would mean:

"unlink socket after successful write" which isn't what we want. Seems to a broken merge. The rest looks good.

review: Needs Fixing

[lp:~schuster/mysql-proxy/remove_unix_socket]

1054. By Michael Schuster on 2010-05-10

mismerge fixed

[Jan Kneschke (jan-kneschke)]

review: Approve

=== modified file 'src/network-address.c'
--- src/network-address.c	2010-04-07 12:44:32 +0000
+++ src/network-address.c	2010-05-10 12:15:45 +0000
@@ -45,6 +45,8 @@
 #include <string.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <glib.h>
+#include <glib/gstdio.h>
 
 #include "network-address.h"
 #include "glib-ext.h"
@@ -63,10 +65,35 @@
 }
 
 void network_address_free(network_address *addr) {
+
 	if (!addr) return;
 
+#ifndef WIN32
+	/*
+	 * if the name we're freeing starts with a '/', we're
+	 * looking at a unix socket which needs to be removed
+	 */
+	if (addr->can_unlink_socket == TRUE && addr->name != NULL &&
+			addr->name->str != NULL) {
+		gchar	*name;
+		int		ret;
+
+		name = addr->name->str;
+		if (name[0] == '/') {
+			ret = g_remove(name);
+			if (ret == 0) {
+				g_debug("%s: removing socket %s successful", 
+					G_STRLOC, name);
+			} else {
+				if (errno != EPERM && errno != EACCES)
+					g_critical("%s: removing socket %s failed: %s (%d)", 
+						G_STRLOC, name, strerror(errno));
+			}
+		}
+	}
+#endif /* WIN32 */
+
 	g_string_free(addr->name, TRUE);
-
 	g_free(addr);
 }
 

=== modified file 'src/network-address.h'
--- src/network-address.h	2010-04-06 14:26:51 +0000
+++ src/network-address.h	2010-05-10 12:15:45 +0000
@@ -63,8 +63,8 @@
 	} addr;
 
 	GString *name; 
-
 	network_socklen_t len;
+	gboolean can_unlink_socket; /* set TRUE *only* after successful bind */
 } network_address;
 
 NETWORK_API network_address *network_address_new(void);

=== modified file 'src/network-socket.c'
--- src/network-socket.c	2010-05-03 14:26:32 +0000
+++ src/network-socket.c	2010-05-10 12:15:45 +0000
@@ -431,6 +431,7 @@
 		}
 	}
 
+	con->dst->can_unlink_socket = TRUE;
 	return NETWORK_SOCKET_SUCCESS;
 }
 

=== modified file 'tests/unit/t_network_socket.c'
--- tests/unit/t_network_socket.c	2010-04-07 12:44:32 +0000
+++ tests/unit/t_network_socket.c	2010-05-10 12:15:45 +0000
@@ -23,6 +23,12 @@
 #include <string.h>
 #include <errno.h>
 
+#ifndef WIN32
+#include <unistd.h>
+#include <stdlib.h>
+#include <fcntl.h>
+#endif /* WIN32 */
+
 #include <glib.h>
 #include <glib/gstdio.h> /* for g_unlink */
 
@@ -31,6 +37,17 @@
 #if GLIB_CHECK_VERSION(2, 16, 0)
 #define C(x) x, sizeof(x) - 1
 
+#ifndef WIN32
+#define LOCAL_SOCK "/tmp/mysql-proxy-test.socket"
+
+typedef struct {
+	char	sockname[sizeof(LOCAL_SOCK) + 10];
+} local_unix_t;
+
+static local_unix_t	*pp = NULL;
+static local_unix_t local_test_arg;
+#endif /* WIN32 */
+
 void test_network_socket_new() {
 	network_socket *sock;
 
@@ -295,6 +312,8 @@
 	network_socket_free(server);
 }
 
+#ifndef WIN32
+
 /**
  * test if _is_local() works on unix-domain sockets
  *
@@ -314,6 +333,7 @@
 	c_sock = network_socket_new();
 	network_address_set_address(c_sock->dst, "/tmp/mysql-proxy-test.socket");
 
+
 	/* hack together a network_socket_accept() which we don't have in this tree yet */
 	g_assert_cmpint(NETWORK_SOCKET_SUCCESS, ==, network_socket_bind(s_sock));
 
@@ -331,6 +351,76 @@
 	g_unlink("/tmp/mysql-proxy-test.socket");
 }
 
+void t_network_localsocket_setup(local_unix_t *p) {
+	g_assert(p != NULL);
+	snprintf(p->sockname, sizeof(p->sockname), LOCAL_SOCK ".%d", (int)getpid());
+	pp = p;
+}
+
+void t_network_localsocket_teardown(local_unix_t *p) {
+	g_assert(p != NULL);
+	if (p->sockname[0] != '\0') {
+		(void) g_unlink(p->sockname);
+		p->sockname[0] = '\0';
+	}
+	pp = NULL;
+}
+
+void exitfunc(int sig) {
+	if (pp != NULL && pp->sockname[0] != '\0')
+		(void) g_unlink(pp->sockname);
+
+	abort();
+}
+
+/**
+ * test if local sockets are removed at shutdown
+ * this is an extension of _is_local_unix(), therefore looks much like it;
+ * just in case, we leave all tests from that function in as well.
+ */
+void t_network_socket_rem_local_unix(local_unix_t *p) {
+	network_socket *s_sock; /* the server side socket, listening for requests */
+	network_socket *c_sock; /* the client side socket, that connects */
+	network_socket *a_sock; /* the server side, accepted socket */
+
+	/*
+	 * if an assertion fails, we receive the ABORT signal. We need to
+	 * close the unix socket in that case too. The regular teardown function
+	 * is not called in this case, so we install our own handler.
+	 */
+	signal(SIGABRT, exitfunc);
+	g_test_bug("42220");
+	g_log_set_always_fatal(G_LOG_FATAL_MASK); /* gtest modifies the fatal-mask */
+
+	s_sock = network_socket_new();
+	network_address_set_address(s_sock->dst, p->sockname);
+
+	c_sock = network_socket_new();
+	network_address_set_address(c_sock->dst, p->sockname);
+
+	g_assert_cmpint(NETWORK_SOCKET_SUCCESS, ==, network_socket_bind(s_sock));
+
+	g_assert_cmpint(g_access(p->sockname, 0), ==, 0);
+
+	g_assert_cmpint(NETWORK_SOCKET_SUCCESS, ==, network_socket_connect(c_sock));
+
+	a_sock = network_socket_accept(s_sock);
+	g_assert(a_sock);
+
+	g_assert_cmpint(TRUE, ==, network_address_is_local(s_sock->dst, a_sock->dst));
+
+	network_socket_free(a_sock);
+	g_assert_cmpint(g_access(p->sockname, 0), ==, 0);
+	network_socket_free(c_sock);
+	g_assert_cmpint(g_access(p->sockname, 0), ==, 0);
+	network_socket_free(s_sock);
+	g_assert_cmpint(g_access(p->sockname, 0), ==, -1);
+
+	/* re-establish default signal disposition */
+	signal(SIGABRT, SIG_DFL);
+}
+
+#endif /* WIN32 */
 
 int main(int argc, char **argv) {
 	g_test_init(&argc, &argv, NULL);
@@ -342,7 +432,14 @@
 	g_test_add_func("/core/network_queue_append", test_network_queue_append);
 	g_test_add_func("/core/network_queue_peek_string", test_network_queue_peek_string);
 	g_test_add_func("/core/network_queue_pop_string", test_network_queue_pop_string);
-	g_test_add_func("/core/network_socket_is_local_unix", t_network_socket_is_local_unix);
+#ifndef WIN32
+	g_test_add_func("/core/network_socket_is_local_unix",t_network_socket_is_local_unix);
+
+	g_test_add("/core/network_socket_rem_local_unix", local_unix_t,
+			&local_test_arg,
+			t_network_localsocket_setup, t_network_socket_rem_local_unix, 
+			t_network_localsocket_teardown);
+#endif /* WIN32 */
 #if 0
 	/**
 	 * disabled for now until we fixed the _to_read() on HP/UX and AIX (and MacOS X)
@@ -354,8 +451,8 @@
 
 	return g_test_run();
 }
-#else
+#else /* GLIB_CHECK_VERSION */
 int main() {
 	return 77;
 }
-#endif
+#endif /* GLIB_CHECK_VERSION */