| Message ID | 1585162176-84776-1-git-send-email-u9012063@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,PATCHv2] fatal-signal: Remove snprintf. | expand |
Thanks, looks good to me. Tested-by: Yifeng Sun <pkusunyifeng@gmail.com> Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> On Wed, Mar 25, 2020 at 11:51 AM William Tu <u9012063@gmail.com> 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/666875084 > Signed-off-by: William Tu <u9012063@gmail.com> > --- > v2: > - avoid strcat overflow buffer, switch to use strncat > - some code refactor > > --- > lib/fatal-signal.c | 45 +++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 37 insertions(+), 8 deletions(-) > > diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c > index 51cf628d994e..e033f1ec59ec 100644 > --- a/lib/fatal-signal.c > +++ b/lib/fatal-signal.c > @@ -158,6 +158,23 @@ 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, res; > + > + if (value / 16 > 0) { > + i = llong_to_hex_str(value / 16, str); > + } > + > + res = value % 16; > + str[i] = "0123456789abcdef"[res]; > + > + return i + 1; > +} > + > /* Send the backtrace buffer to monitor thread. > * > * Note that this runs in the signal handling context, any system > @@ -192,20 +209,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); > + > + strcat(line, "0x"); > + strcat(line, ip_str); > + strcat(line, "<"); > + strncat(line, unw_bt[i].func, UNW_MAX_FUNCN); > + strcat(line, "+0x"); > + strcat(line, offset_str); > + strcat(line, ">\n"); > vlog_direct_write_to_log_file_unsafe(line); > } > } > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Apr 13, 2020 at 3:37 PM Yifeng Sun <pkusunyifeng@gmail.com> wrote: > > Thanks, looks good to me. > > Tested-by: Yifeng Sun <pkusunyifeng@gmail.com> > > Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> > > > On Wed, Mar 25, 2020 at 11:51 AM William Tu <u9012063@gmail.com> 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/666875084 >> Signed-off-by: William Tu <u9012063@gmail.com> >> --- >> v2: >> - avoid strcat overflow buffer, switch to use strncat >> - some code refactor >> >> --- >> lib/fatal-signal.c | 45 +++++++++++++++++++++++++++++++++++++-------- >> 1 file changed, 37 insertions(+), 8 deletions(-) >> >> diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c >> index 51cf628d994e..e033f1ec59ec 100644 >> --- a/lib/fatal-signal.c >> +++ b/lib/fatal-signal.c >> @@ -158,6 +158,23 @@ 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, res; >> + >> + if (value / 16 > 0) { >> + i = llong_to_hex_str(value / 16, str); >> + } >> + >> + res = value % 16; >> + str[i] = "0123456789abcdef"[res]; >> + >> + return i + 1; >> +} >> + >> /* Send the backtrace buffer to monitor thread. >> * >> * Note that this runs in the signal handling context, any system >> @@ -192,20 +209,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); >> + >> + strcat(line, "0x"); >> + strcat(line, ip_str); >> + strcat(line, "<"); >> + strncat(line, unw_bt[i].func, UNW_MAX_FUNCN); I tested with gcc10 and got an error from ./lib/string.h:20, from lib/fatal-signal.c:25: lib/fatal-signal.c: In function ‘send_backtrace_to_monitor’: lib/fatal-signal.c:234:13: warning: ‘__builtin_strncat’ output may be truncated copying 32 bytes from a string of length 1535 [ ]8;;https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wstringop-truncation-Wstringop-truncation ]8;;] 234 | strncat(line, unw_bt[i].func, UNW_MAX_FUNCN); | ^~~~~~~ I'm going to fix it and send another version William
diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c index 51cf628d994e..e033f1ec59ec 100644 --- a/lib/fatal-signal.c +++ b/lib/fatal-signal.c @@ -158,6 +158,23 @@ 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, res; + + if (value / 16 > 0) { + i = llong_to_hex_str(value / 16, str); + } + + res = value % 16; + str[i] = "0123456789abcdef"[res]; + + return i + 1; +} + /* Send the backtrace buffer to monitor thread. * * Note that this runs in the signal handling context, any system @@ -192,20 +209,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); + + strcat(line, "0x"); + strcat(line, ip_str); + strcat(line, "<"); + strncat(line, unw_bt[i].func, UNW_MAX_FUNCN); + strcat(line, "+0x"); + strcat(line, offset_str); + 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/666875084 Signed-off-by: William Tu <u9012063@gmail.com> --- v2: - avoid strcat overflow buffer, switch to use strncat - some code refactor --- lib/fatal-signal.c | 45 +++++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-)