[ovs-dev] [PATCHv2] fatal-signal: Remove snprintf.

[William Tu]

On Mon, Apr 13, 2020 at 3:37 PM Yifeng Sun <pkusunyifeng at gmail.com> wrote:
>
> Thanks, looks good to me.
>
> Tested-by: Yifeng Sun <pkusunyifeng at gmail.com>
>
> Reviewed-by: Yifeng Sun <pkusunyifeng at gmail.com>
>
>
> On Wed, Mar 25, 2020 at 11:51 AM William Tu <u9012063 at 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 at 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