From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp36.i.mail.ru (smtp36.i.mail.ru [94.100.177.96]) (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 143E9469719 for ; Tue, 3 Nov 2020 16:59:44 +0300 (MSK) Date: Tue, 3 Nov 2020 16:59:42 +0300 From: Sergey Ostanevich Message-ID: <20201103135942.GD517@tarantool.org> References: <429b3e4d-490d-ea88-946a-fc0487f9e46a@tarantool.org> <20201102131945.GB517@tarantool.org> <20201102211828.GE2339@grain> <20201102214720.GF2339@grain> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20201102214720.GF2339@grain> 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: Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy 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 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 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.