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

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

[Sergey Ostanevich]

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);
              }
              addrlen = sizeof(addr);
              rc = getpeername(fd, (struct sockaddr *) &addr, &addrlen);
-- 
2.24.3 (Apple Git-128)

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

It seems the patch fixes the bug. I don't have crashes anymore.

LGTM.

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

[Vladislav Shpilevoy]

If there won't be a second review in a few days, I will
commit this. I can't properly run tests without this patch.

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

[Cyrill Gorcunov]

[-- Attachment #1: Type: text/plain, Size: 216 bytes --]

Ack

On Fri, Oct 30, 2020 at 1:56 AM Vladislav Shpilevoy <
v.shpilevoy@tarantool.org> wrote:

> If there won't be a second review in a few days, I will
> commit this. I can't properly run tests without this patch.
>

[-- Attachment #2: Type: text/html, Size: 512 bytes --]

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

[Vladislav Shpilevoy]

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().

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

[Sergey Ostanevich]

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().

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

[Vladislav Shpilevoy]

> 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.

Woops, SNPRINT is used a lot in the code. If it is true, we need to fix SNPRINT.

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

With your patch it is also an issue with alignment. See below. I
tried to fix it in-place, but it was too ugly due to too big
indentation.

> --- 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));

Here sio_strfaddr( should get +17 spaces, and that makes it hard
to read. So I extracted everything to a new function with lower
indentation.

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

[Cyrill Gorcunov]

On Mon, Nov 02, 2020 at 10:09:29PM +0100, Vladislav Shpilevoy wrote:
> > 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.
> 
> Woops, SNPRINT is used a lot in the code. If it is true, we need to fix SNPRINT.

Guys, I didn't follow the details of the series but thought of some
helper like below. Will it help?
---
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Sun, 1 Nov 2020 16:10:08 +0300
Subject: [PATCH] util: introduce scnprintf helper

The misuse of snprintf is pretty common, lets
provide scnprintf helper which is just a wrapper
over vsnprintf with more sane return.

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---
 src/lib/core/util.c | 24 ++++++++++++++++++++++++
 src/trivia/util.h   |  3 +++
 2 files changed, 27 insertions(+)

diff --git a/src/lib/core/util.c b/src/lib/core/util.c
index dfce317f0..60e552174 100644
--- a/src/lib/core/util.c
+++ b/src/lib/core/util.c
@@ -84,6 +84,30 @@ strnindex(const char **haystack, const char *needle, uint32_t len, uint32_t hmax
 	return hmax;
 }
 
+/**
+ * scnprintf - Put formatted string into a buffer
+ *
+ * @param buf The destination buffer
+ * @param size The size of the buffer
+ * @param fmt The format string
+ * @param ... Arguments
+ *
+ * @return Number of chars written into \a buf without ending '\0'.
+ * @return 0 if \a size if 0.
+ */
+int
+scnprintf(char *buf, size_t size, const char *fmt, ...)
+{
+	va_list args;
+	int i;
+
+	va_start(args, fmt);
+	i = vsnprintf(buf, size, fmt, args);
+	va_end(args);
+
+	return i < (int)size ? i : ((size != 0) ? (int)size-1 : 0);
+}
+
 void
 close_all_xcpt(int fdc, ...)
 {
diff --git a/src/trivia/util.h b/src/trivia/util.h
index da5a3705e..cf2da15fc 100644
--- a/src/trivia/util.h
+++ b/src/trivia/util.h
@@ -98,6 +98,9 @@ strindex(const char **haystack, const char *needle, uint32_t hmax);
 uint32_t
 strnindex(const char **haystack, const char *needle, uint32_t len, uint32_t hmax);
 
+int
+scnprintf(char *buf, size_t size, const char *fmt, ...);
+
 #define nelem(x)     (sizeof((x))/sizeof((x)[0]))
 #define field_sizeof(compound_type, field) sizeof(((compound_type *)NULL)->field)
 #ifndef lengthof
-- 
2.26.2

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

[Vladislav Shpilevoy]

On 02.11.2020 22:18, Cyrill Gorcunov wrote:
> On Mon, Nov 02, 2020 at 10:09:29PM +0100, Vladislav Shpilevoy wrote:
>>> 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.
>>
>> Woops, SNPRINT is used a lot in the code. If it is true, we need to fix SNPRINT.
> 
> 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.

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

[Cyrill Gorcunov]

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.

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

[Sergey Ostanevich]

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.

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

[Cyrill Gorcunov]

On Tue, Nov 03, 2020 at 04:59:42PM +0300, Sergey Ostanevich wrote:
> 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.

Looks ok to me.

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

[Vladislav Shpilevoy]

Pushed to 1.10, 2.5, 2.6, and master.