[ovs-dev] fatal-signal: Remove snprintf.
diff mbox series

Message ID 1585146853-129779-1-git-send-email-u9012063@gmail.com
State New
Headers show
Series
  • [ovs-dev] fatal-signal: Remove snprintf.
Related show

Commit Message

William Tu March 25, 2020, 2:34 p.m. UTC
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(-)

Comments

Ben Pfaff March 25, 2020, 3:38 p.m. UTC | #1
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");
William Tu March 25, 2020, 3:52 p.m. UTC | #2
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

Patch
diff mbox series

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);
         }
     }