From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 5991246970F for ; Tue, 26 Nov 2019 15:09:03 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) From: Serge Petrenko In-Reply-To: Date: Tue, 26 Nov 2019 15:09:01 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <7524BAB0-1441-437A-A07E-2693F80D751D@tarantool.org> References: <20191125162701.82576-1-sergepetrenko@tarantool.org> 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: Vladislav Shpilevoy Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for review! > 26 =D0=BD=D0=BE=D1=8F=D0=B1. 2019 =D0=B3., =D0=B2 2:02, Vladislav = Shpilevoy =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0= =D0=BB(=D0=B0): >=20 > Hi! Thanks for the patch! >=20 > 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 =3D 0, but do it before the > cycle. True, here=E2=80=99s 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 =3D backtrace_buf; char *end =3D p + sizeof(backtrace_buf) - 1; int unw_status; + *p =3D '\0'; while ((unw_status =3D unw_step(&unw_cur)) > 0) { const char *proc; old_sp =3D sp; @@ -173,7 +174,6 @@ backtrace() say_debug("unwinding error: %i", unw_status); #endif out: - *p =3D '\0'; return backtrace_buf; } =20 --=20 2.21.0 (Apple Git-122) =20 >=20 > On 25/11/2019 17:27, Serge Petrenko wrote: >> 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. >>=20 >> Closes #4636 >> --- >> https://github.com/tarantool/tarantool/issues/4636 >> https://github.com/tarantool/tarantool/tree/sp/gh-4636-bt-print-fix >>=20 >> src/lib/core/backtrace.cc | 1 - >> 1 file changed, 1 deletion(-) >>=20 >> diff --git a/src/lib/core/backtrace.cc b/src/lib/core/backtrace.cc >> index 57e541c25..903ffb79c 100644 >> --- a/src/lib/core/backtrace.cc >> +++ b/src/lib/core/backtrace.cc >> @@ -173,7 +173,6 @@ backtrace() >> say_debug("unwinding error: %i", unw_status); >> #endif >> out: >> - *p =3D '\0'; >> return backtrace_buf; >> } >>=20 >>=20 -- Serge Petrenko sergepetrenko@tarantool.org