| Message ID | 1585146853-129779-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] fatal-signal: Remove snprintf. | expand |
On Wed, Mar 25, 2020 at 07:34:13AM -0700, William Tu wrote: > Function snprintf is not async-signal-safe. Replace it with > our own implementation. Example ovs-vswitchd.log output: > 2020-03-25T01:08:19.673Z|00050|memory|INFO|handlers:2 ports:3 > SIGSEGV detected, backtrace: > 0x4872d9 <fatal_signal_handler+0x49> > 0x7f4e2ab974b0 <killpg+0x40> > 0x7f4e2ac5d74d <__poll+0x2d> > 0x531098 <time_poll+0x108> > 0x51aefc <poll_block+0x8c> > 0x445ca9 <udpif_revalidator+0x289> > 0x5056fd <ovsthread_wrapper+0x7d> > 0x7f4e2b65f6ba <start_thread+0xca> > 0x7f4e2ac6941d <clone+0x6d> > 0x0 <+0x0> > > Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666596271 > Signed-off-by: William Tu <u9012063@gmail.com> I didn't test this, but I assume you did. The traditional way to get a hex digit is to write "0123456789abcdef"[x], although your way is fine too. If the function name is long, this will overflow the line buffer. There's no reason to write "line =" in each of these, since strcat() just returns its first argument. > + line = strcat(line, "0x"); > + line = strcat(line, ip_str); > + line = strcat(line, "<"); > + line = strcat(line, unw_bt[i].func); > + line = strcat(line, "+0x"); > + line = strcat(line, offset_str); > + line = strcat(line, ">\n");
On Wed, Mar 25, 2020 at 8:39 AM Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Mar 25, 2020 at 07:34:13AM -0700, William Tu wrote: > > Function snprintf is not async-signal-safe. Replace it with > > our own implementation. Example ovs-vswitchd.log output: > > 2020-03-25T01:08:19.673Z|00050|memory|INFO|handlers:2 ports:3 > > SIGSEGV detected, backtrace: > > 0x4872d9 <fatal_signal_handler+0x49> > > 0x7f4e2ab974b0 <killpg+0x40> > > 0x7f4e2ac5d74d <__poll+0x2d> > > 0x531098 <time_poll+0x108> > > 0x51aefc <poll_block+0x8c> > > 0x445ca9 <udpif_revalidator+0x289> > > 0x5056fd <ovsthread_wrapper+0x7d> > > 0x7f4e2b65f6ba <start_thread+0xca> > > 0x7f4e2ac6941d <clone+0x6d> > > 0x0 <+0x0> > > > > Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666596271 > > Signed-off-by: William Tu <u9012063@gmail.com> > > I didn't test this, but I assume you did. > > The traditional way to get a hex digit is to write "0123456789abcdef"[x], > although your way is fine too. Thanks! I think this looks better. > > If the function name is long, this will overflow the line buffer. OK, I will use strncat > > There's no reason to write "line =" in each of these, since strcat() > just returns its first argument. > OK, will fix it. Thanks for your feedback. William
diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c index 51cf628d994e..52794a0722cb 100644 --- a/lib/fatal-signal.c +++ b/lib/fatal-signal.c @@ -158,6 +158,27 @@ fatal_signal_add_hook(void (*hook_cb)(void *aux), void (*cancel_cb)(void *aux), } #ifdef HAVE_UNWIND +/* Convert unsigned long long to string. This is needed because + * using snprintf() is not async signal safe. */ +static inline int +llong_to_hex_str(unsigned long long value, char *str) +{ + int i = 0; + char res; + + if (value / 16 > 0) { + i = llong_to_hex_str(value / 16, str); + } + + res = (char)(value % 16); + if (res < 10) { + str[i] = '0' + res; + } else { + str[i] = 'a' + res - 10; + } + return i + 1; +} + /* Send the backtrace buffer to monitor thread. * * Note that this runs in the signal handling context, any system @@ -192,20 +213,32 @@ send_backtrace_to_monitor(void) { dep * sizeof(struct unw_backtrace))); } else { /* Since there is no monitor daemon running, write backtrace - * in current process. This is not asyn-signal-safe due to - * use of snprintf(). + * in current process. */ char str[] = "SIGSEGV detected, backtrace:\n"; + char ip_str[16], offset_str[6]; + char _line[64]; + char *line = (char *)_line; vlog_direct_write_to_log_file_unsafe(str); for (int i = 0; i < dep; i++) { - char line[64]; - - snprintf(line, 64, "0x%016"PRIxPTR" <%s+0x%"PRIxPTR">\n", - unw_bt[i].ip, - unw_bt[i].func, - unw_bt[i].offset); + memset(line, 0, sizeof _line); + memset(ip_str, ' ', sizeof ip_str); + memset(offset_str, 0, sizeof offset_str); + ip_str[sizeof(ip_str) - 1] = 0; + offset_str[sizeof(offset_str) - 1] = 0; + + llong_to_hex_str(unw_bt[i].ip, ip_str); + llong_to_hex_str(unw_bt[i].offset, offset_str); + + line = strcat(line, "0x"); + line = strcat(line, ip_str); + line = strcat(line, "<"); + line = strcat(line, unw_bt[i].func); + line = strcat(line, "+0x"); + line = strcat(line, offset_str); + line = strcat(line, ">\n"); vlog_direct_write_to_log_file_unsafe(line); } }
Function snprintf is not async-signal-safe. Replace it with our own implementation. Example ovs-vswitchd.log output: 2020-03-25T01:08:19.673Z|00050|memory|INFO|handlers:2 ports:3 SIGSEGV detected, backtrace: 0x4872d9 <fatal_signal_handler+0x49> 0x7f4e2ab974b0 <killpg+0x40> 0x7f4e2ac5d74d <__poll+0x2d> 0x531098 <time_poll+0x108> 0x51aefc <poll_block+0x8c> 0x445ca9 <udpif_revalidator+0x289> 0x5056fd <ovsthread_wrapper+0x7d> 0x7f4e2b65f6ba <start_thread+0xca> 0x7f4e2ac6941d <clone+0x6d> 0x0 <+0x0> Tested-at: https://travis-ci.org/github/williamtu/ovs-travis/builds/666596271 Signed-off-by: William Tu <u9012063@gmail.com> --- lib/fatal-signal.c | 49 +++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 41 insertions(+), 8 deletions(-)