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

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Thanks for the investigation! My bad, I used MIN as a function with
sematics of all agruments calculated before call. You're right - in case
of define it can cause a double call.

The SNPRINT although leaves some questions to me: in case 'written' is
more or equal to 'size', it forces '_buf' to be set to NULL. But in the
sio_socketname_to_buffer() there's no check for NULL between the calls.
A call to snprintf() delivers a segfault, at least for Mac.

Also, factoring out 18 loc out of a function with a total of 24 seems
redundant - IMVHO. 

I prpose to explicitly get the result from snprintf and compare it to
original limit. 

======
--- a/src/lib/core/sio.c
+++ b/src/lib/core/sio.c
@@ -53,20 +53,22 @@ 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 to_be_printed = snprintf(name, name_size, "fd %d", fd);
+       int n = MIN(to_be_printed, 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",
+                       to_be_printed = snprintf(name + n, name_size - n, ", aka %s",
                                sio_strfaddr((struct sockaddr *)&addr,
                                                                addrlen));
+                       n += MIN(to_be_printed, name_size - n);
                }
                addrlen = sizeof(addr);
                rc = getpeername(fd, (struct sockaddr *) &addr, &addrlen);
                if (rc == 0) {
-                       n += snprintf(name + n, name_size - n,
+                       to_be_printed = snprintf(name + n, name_size - n,
                                      ", peer of %s",
                                      sio_strfaddr((struct sockaddr *)&addr,
                                                                addrlen));



On 31 окт 17:33, Vladislav Shpilevoy wrote:
> 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().