Re: [Tarantool-patches] [PATCH] core: fix static_alloc buffer overflow

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

On 23.10.2020 17:13, Sergey Ostanevich wrote:
> Static buffer overflow in thread local pool causes random fails on OSX
> platform. This was caused by an incorrect use of the allocator result:
> the snprintf returns the full size of the formatted string, rather the
> number of bytes written.
> 
> Fixes #5312
> 
> Branch: https://github.com/tarantool/tarantool/tree/sergos/gh-5312-crash-in-libeio
> Issue: https://github.com/tarantool/tarantool/issues/5312
> 
> ---
> src/lib/core/sio.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/lib/core/sio.c b/src/lib/core/sio.c
> index 97a512eee..44f952c7c 100644
> --- a/src/lib/core/sio.c
> +++ b/src/lib/core/sio.c
> @@ -53,15 +53,15 @@ sio_socketname(int fd)
>       int save_errno = errno;
>       int name_size = 2 * SERVICE_NAME_MAXLEN;
>       char *name = static_alloc(name_size);
> -       int n = snprintf(name, name_size, "fd %d", fd);
> +       int n = MIN(snprintf(name, name_size, "fd %d", fd), name_size);
>       if (fd >= 0) {
>               struct sockaddr_storage addr;
>               socklen_t addrlen = sizeof(addr);
>               int rc = getsockname(fd, (struct sockaddr *) &addr, &addrlen);
>               if (rc == 0) {
> -                       n += snprintf(name + n, name_size - n, ", aka %s",
> +                       n += MIN(snprintf(name + n, name_size - n, ", aka %s",
>                               sio_strfaddr((struct sockaddr *)&addr,
> -                                                               addrlen));
> +                                                addrlen)), name_size - n);

After thinking more I realized it is not entirely correct.
MIN(a, b) = ((a) < (b) ? (a) : (b)). It means, that either
'a' or 'b' is executed twice. If 'a' is the big snprintf above, 
it is also executed twice. So you can't really use MIN right away.

I reworked the patch to use an existing macro - SNPRINT. See the
new patch below and on the branch in a separate commit.

I added a new function, because SNPRINT returns -1 in case of
print function fail.

====================
diff --git a/src/lib/core/sio.c b/src/lib/core/sio.c
index 44f952c7c..d008526d5 100644
--- a/src/lib/core/sio.c
+++ b/src/lib/core/sio.c
@@ -46,6 +46,33 @@
 static_assert(SMALL_STATIC_SIZE > NI_MAXHOST + NI_MAXSERV,
 	      "static buffer should fit host name");
 
+/**
+ * Safely print a socket description to the given buffer, with correct overflow
+ * checks and all.
+ */
+static int
+sio_socketname_to_buffer(int fd, char *buf, int size)
+{
+	int n = 0;
+	SNPRINT(n, snprintf, buf, size, "fd %d", fd);
+	if (fd < 0)
+		return 0;
+	struct sockaddr_storage addr;
+	socklen_t addrlen = sizeof(addr);
+	int rc = getsockname(fd, (struct sockaddr *) &addr, &addrlen);
+	if (rc == 0) {
+		SNPRINT(n, snprintf, buf, size, ", aka %s",
+			sio_strfaddr((struct sockaddr *)&addr, addrlen));
+	}
+	addrlen = sizeof(addr);
+	rc = getpeername(fd, (struct sockaddr *) &addr, &addrlen);
+	if (rc == 0) {
+		SNPRINT(n, snprintf, buf, size, ", peer of %s",
+			sio_strfaddr((struct sockaddr *)&addr, addrlen));
+	}
+	return 0;
+}
+
 const char *
 sio_socketname(int fd)
 {
@@ -53,25 +80,13 @@ sio_socketname(int fd)
 	int save_errno = errno;
 	int name_size = 2 * SERVICE_NAME_MAXLEN;
 	char *name = static_alloc(name_size);
-	int n = MIN(snprintf(name, name_size, "fd %d", fd), name_size);
-	if (fd >= 0) {
-		struct sockaddr_storage addr;
-		socklen_t addrlen = sizeof(addr);
-		int rc = getsockname(fd, (struct sockaddr *) &addr, &addrlen);
-		if (rc == 0) {
-			n += MIN(snprintf(name + n, name_size - n, ", aka %s",
-				sio_strfaddr((struct sockaddr *)&addr,
-                                                addrlen)), name_size - n);
-		}
-		addrlen = sizeof(addr);
-		rc = getpeername(fd, (struct sockaddr *) &addr, &addrlen);
-		if (rc == 0) {
-			n += snprintf(name + n, name_size - n,
-				      ", peer of %s",
-				      sio_strfaddr((struct sockaddr *)&addr,
-								addrlen));
-		}
-	}
+	int rc = sio_socketname_to_buffer(fd, name, name_size);
+	/*
+	 * Could fail only because of a bad format in snprintf, but it is not
+	 * bad, so should not fail.
+	 */
+	assert(rc == 0);
+	(void)rc;
 	/*
 	 * Restore the original errno, it might have been reset by
 	 * snprintf() or getsockname().