From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 6DFDB46970F for ; Tue, 26 Nov 2019 23:30:27 +0300 (MSK) References: <20191125162701.82576-1-sergepetrenko@tarantool.org> <7524BAB0-1441-437A-A07E-2693F80D751D@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Tue, 26 Nov 2019 21:30:10 +0100 MIME-Version: 1.0 In-Reply-To: <7524BAB0-1441-437A-A07E-2693F80D751D@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko , Kirill Yukhin Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the fix! LGTM. On 26/11/2019 13:09, Serge Petrenko wrote: > Hi! Thank you for review! > >> 26 нояб. 2019 г., в 2:02, Vladislav Shpilevoy написал(а): >> >> Hi! Thanks for the patch! >> >> Perhaps the zero termination was done for a case, >> when the cycle in backtrace() does not run even one >> iteration. For example, if unw_step() returns an >> error. Then the buffer is not terminated. So I think >> it is better to keep *p = 0, but do it before the >> cycle. > > True, here’s the new patch: > > snrpintf always null-terminates the passed string, and it also returns > the number of bytes that "would have been written if there was enough > space", so not only we don't have to null-terminate the string, but even > more so we shouldn't do it erroneously. The only case when a string > should be null-terminated manually is when the print cycle doesn't run > at all, so move the termination before the cycle. > > Closes #4636 > --- > src/lib/core/backtrace.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc > index 57e541c25..77f77b05c 100644 > --- a/src/lib/core/backtrace.cc > +++ b/src/lib/core/backtrace.cc > @@ -143,6 +143,7 @@ backtrace() > char *p = backtrace_buf; > char *end = p + sizeof(backtrace_buf) - 1; > int unw_status; > + *p = '\0'; > while ((unw_status = unw_step(&unw_cur)) > 0) { > const char *proc; > old_sp = sp; > @@ -173,7 +174,6 @@ backtrace() > say_debug("unwinding error: %i", unw_status); > #endif > out: > - *p = '\0'; > return backtrace_buf; > } > >