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

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

I double checked the snprintf fails only if buf is set to NULL _and_ the
len is set to a non-zero value. 

So I would follow Vlad's proposal on factoring out the code to make it
clearer.

Sergos.

====

commit 6f6eb1b8f12679b69eba40ea960a9ebb439c798e
Author: Sergey Ostanevich <sergos@tarantool.org>
Date:   Fri Oct 23 16:09:31 2020 +0300

    core: fix static_alloc buffer overflow
    
    Static buffer overflow in thread local pool causes random fails on OSX
    platform. This was caused by an incorrect use of the allocator result.
    
    Fixes #5312
    
    Co-authored-by: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>

diff --git a/src/lib/core/sio.c b/src/lib/core/sio.c
index 97a512eee..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 = snprintf(name, name_size, "fd %d", fd);
-	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",
-				sio_strfaddr((struct sockaddr *)&addr,
-								addrlen));
-		}
-		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().

On 03 ноя 00:47, Cyrill Gorcunov wrote:
> On Mon, Nov 02, 2020 at 10:43:02PM +0100, Vladislav Shpilevoy wrote:
> > > 
> > > Guys, I didn't follow the details of the series but thought of some
> > > helper like below. Will it help?
> > 
> > In some places yes. But SNPRINT is used not only with snprintf.
> > 
> > It also is used with vsnprintf, mp_snprint, vy_run_snprint_filename,
> > tuple_snprint, say_format_plain_tail, json_escape, strftime, and probably
> > more. So it would be better to fix SNPRINT. To cover all its usage
> > cases.
> 
> OK, sounds reasonable.