From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4F064469719 for ; Sat, 31 Oct 2020 19:33:22 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: <429b3e4d-490d-ea88-946a-fc0487f9e46a@tarantool.org> Date: Sat, 31 Oct 2020 17:33:20 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] core: fix static_alloc buffer overflow List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Sergey Ostanevich , tarantool-patches@dev.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().