* [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing @ 2019-11-25 16:27 Serge Petrenko 2019-11-25 23:02 ` Vladislav Shpilevoy 2019-12-10 14:06 ` Kirill Yukhin 0 siblings, 2 replies; 5+ messages in thread From: Serge Petrenko @ 2019-11-25 16:27 UTC (permalink / raw) To: v.shpilevoy; +Cc: tarantool-patches 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. Closes #4636 --- https://github.com/tarantool/tarantool/issues/4636 https://github.com/tarantool/tarantool/tree/sp/gh-4636-bt-print-fix src/lib/core/backtrace.cc | 1 - 1 file changed, 1 deletion(-) 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 = '\0'; return backtrace_buf; } -- 2.21.0 (Apple Git-122) ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing 2019-11-25 16:27 [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing Serge Petrenko @ 2019-11-25 23:02 ` Vladislav Shpilevoy 2019-11-26 12:09 ` Serge Petrenko 2019-12-10 14:06 ` Kirill Yukhin 1 sibling, 1 reply; 5+ messages in thread From: Vladislav Shpilevoy @ 2019-11-25 23:02 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches 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. 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. > > Closes #4636 > --- > https://github.com/tarantool/tarantool/issues/4636 > https://github.com/tarantool/tarantool/tree/sp/gh-4636-bt-print-fix > > src/lib/core/backtrace.cc | 1 - > 1 file changed, 1 deletion(-) > > 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 = '\0'; > return backtrace_buf; > } > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing 2019-11-25 23:02 ` Vladislav Shpilevoy @ 2019-11-26 12:09 ` Serge Petrenko 2019-11-26 20:30 ` Vladislav Shpilevoy 0 siblings, 1 reply; 5+ messages in thread From: Serge Petrenko @ 2019-11-26 12:09 UTC (permalink / raw) To: Vladislav Shpilevoy; +Cc: tarantool-patches Hi! Thank you for review! > 26 нояб. 2019 г., в 2:02, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а): > > 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; } -- 2.21.0 (Apple Git-122) > > 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. >> >> Closes #4636 >> --- >> https://github.com/tarantool/tarantool/issues/4636 >> https://github.com/tarantool/tarantool/tree/sp/gh-4636-bt-print-fix >> >> src/lib/core/backtrace.cc | 1 - >> 1 file changed, 1 deletion(-) >> >> 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 = '\0'; >> return backtrace_buf; >> } >> >> -- Serge Petrenko sergepetrenko@tarantool.org ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing 2019-11-26 12:09 ` Serge Petrenko @ 2019-11-26 20:30 ` Vladislav Shpilevoy 0 siblings, 0 replies; 5+ messages in thread From: Vladislav Shpilevoy @ 2019-11-26 20:30 UTC (permalink / raw) To: Serge Petrenko, Kirill Yukhin; +Cc: tarantool-patches 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 <v.shpilevoy@tarantool.org> написал(а): >> >> 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; > } > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing 2019-11-25 16:27 [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing Serge Petrenko 2019-11-25 23:02 ` Vladislav Shpilevoy @ 2019-12-10 14:06 ` Kirill Yukhin 1 sibling, 0 replies; 5+ messages in thread From: Kirill Yukhin @ 2019-12-10 14:06 UTC (permalink / raw) To: Serge Petrenko; +Cc: tarantool-patches, v.shpilevoy Hello, On 25 ноя 19: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. > > Closes #4636 > --- > https://github.com/tarantool/tarantool/issues/4636 > https://github.com/tarantool/tarantool/tree/sp/gh-4636-bt-print-fix I've checked your patch into master, 2.2 and 1.10. -- Regards, Kirill Yukhin ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-10 14:06 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-25 16:27 [Tarantool-patches] [PATCH] backtrace: fix out of bounds access on backtrace printing Serge Petrenko 2019-11-25 23:02 ` Vladislav Shpilevoy 2019-11-26 12:09 ` Serge Petrenko 2019-11-26 20:30 ` Vladislav Shpilevoy 2019-12-10 14:06 ` Kirill Yukhin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox