From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 0C343469719 for ; Mon, 2 Nov 2020 16:19:46 +0300 (MSK) Date: Mon, 2 Nov 2020 16:19:45 +0300 From: Sergey Ostanevich Message-ID: <20201102131945.GB517@tarantool.org> References: <429b3e4d-490d-ea88-946a-fc0487f9e46a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <429b3e4d-490d-ea88-946a-fc0487f9e46a@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.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().