Bug 80074 - missing -Wstringop-overflow on a detected __builtin___sprintf_chk overflow
Summary: missing -Wstringop-overflow on a detected __builtin___sprintf_chk overflow
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: 7.0
Assignee: Martin Sebor
URL:
Keywords: diagnostic
Depends on:
Blocks: Wstringop-overflow
  Show dependency treegraph
 
Reported: 2017-03-16 22:23 UTC by Martin Sebor
Modified: 2020-04-22 21:08 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail: 7.1.0
Last reconfirmed: 2017-03-16 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Sebor 2017-03-16 22:23:12 UTC
The call to __builtin___sprintf_chk in the following program overflows the destination.  The overflow is detected at runtime and is clearly detectable at compile-time, but it's not diagnosed as it should be.

$ cat t.c && gcc -Wall -Wextra -Wpedantic -Wrestrict -fdump-tree-optimized=/dev/stdout t.c && ./a.out 
char d[1];

int main (int argc, char *argv[])
{
  const char *s = argc < 0 ? "123" : "456";

  int n = __builtin___sprintf_chk (d, 0, sizeof d, "%s", s);

  __builtin_printf ("%i: \"%.*s\"\n", n, n, d);
}
t.c: In function ‘main’:
t.c:3:27: warning: unused parameter ‘argv’ [-Wunused-parameter]
 int main (int argc, char *argv[])
                           ^~~~

;; Function main (main, funcdef_no=0, decl_uid=1797, cgraph_uid=0, symbol_order=1)

main (int argc, char * * argv)
{
  int n;
  const char * s;
  int D.1806;
  char[4] * iftmp.0;
  char[4] * iftmp.0_1;
  char[4] * iftmp.0_3;
  char[4] * iftmp.0_4;
  int _10;

  <bb 2> [0.00%]:
  if (argc_2(D) < 0)
    goto <bb 3>; [0.00%]
  else
    goto <bb 4>; [0.00%]

  <bb 3> [0.00%]:
  iftmp.0_4 = "123";
  goto <bb 5>; [0.00%]

  <bb 4> [0.00%]:
  iftmp.0_3 = "456";

  <bb 5> [0.00%]:
  # iftmp.0_1 = PHI <iftmp.0_4(3), iftmp.0_3(4)>
  s_5 = iftmp.0_1;
  n_8 = __builtin___sprintf_chk (&d, 0, 1, "%s", s_5);
  __builtin_printf ("%i: \"%.*s\"\n", n_8, n_8, &d);
  _10 = 0;

<L3> [0.00%]:
  return _10;

}


*** buffer overflow detected ***: ./a.out terminated
======= Backtrace: =========
/lib64/libc.so.6(+0x77de5)[0x7f0975f73de5]
/lib64/libc.so.6(__fortify_fail+0x37)[0x7f0976010167]
/lib64/libc.so.6(+0x1122f0)[0x7f097600e2f0]
/lib64/libc.so.6(+0x111859)[0x7f097600d859]
/lib64/libc.so.6(_IO_default_xsputn+0x80)[0x7f0975f77bc0]
/lib64/libc.so.6(_IO_vfprintf+0xaa6)[0x7f0975f48336]
/lib64/libc.so.6(__vsprintf_chk+0x8c)[0x7f097600d8ec]
/lib64/libc.so.6(__sprintf_chk+0x7d)[0x7f097600d83d]
./a.out[0x4005fb]
/lib64/libc.so.6(__libc_start_main+0xf0)[0x7f0975f1c580]
./a.out[0x4004c9]
======= Memory map: ========
00400000-00401000 r-xp 00000000 fd:03 8129719                            /home/msebor/build/tmp/a.out
00600000-00601000 r--p 00000000 fd:03 8129719                            /home/msebor/build/tmp/a.out
00601000-00602000 rw-p 00001000 fd:03 8129719                            /home/msebor/build/tmp/a.out
00a07000-00a28000 rw-p 00000000 00:00 0                                  [heap]
7f0975ce5000-7f0975cfb000 r-xp 00000000 fd:00 4091                       /usr/lib64/libgcc_s-5.3.1-20160406.so.1
7f0975cfb000-7f0975efa000 ---p 00016000 fd:00 4091                       /usr/lib64/libgcc_s-5.3.1-20160406.so.1
7f0975efa000-7f0975efb000 r--p 00015000 fd:00 4091                       /usr/lib64/libgcc_s-5.3.1-20160406.so.1
7f0975efb000-7f0975efc000 rw-p 00016000 fd:00 4091                       /usr/lib64/libgcc_s-5.3.1-20160406.so.1
7f0975efc000-7f09760b3000 r-xp 00000000 fd:00 11035                      /usr/lib64/libc-2.22.so
7f09760b3000-7f09762b3000 ---p 001b7000 fd:00 11035                      /usr/lib64/libc-2.22.so
7f09762b3000-7f09762b7000 r--p 001b7000 fd:00 11035                      /usr/lib64/libc-2.22.so
7f09762b7000-7f09762b9000 rw-p 001bb000 fd:00 11035                      /usr/lib64/libc-2.22.so
7f09762b9000-7f09762bd000 rw-p 00000000 00:00 0 
7f09762bd000-7f09762de000 r-xp 00000000 fd:00 73564                      /usr/lib64/ld-2.22.so
7f09764c1000-7f09764c4000 rw-p 00000000 00:00 0 
7f09764db000-7f09764dd000 rw-p 00000000 00:00 0 
7f09764dd000-7f09764de000 r--p 00020000 fd:00 73564                      /usr/lib64/ld-2.22.so
7f09764de000-7f09764df000 rw-p 00021000 fd:00 73564                      /usr/lib64/ld-2.22.so
7f09764df000-7f09764e0000 rw-p 00000000 00:00 0 
7ffd4b095000-7ffd4b0b6000 rw-p 00000000 00:00 0                          [stack]
7ffd4b1a3000-7ffd4b1a5000 r--p 00000000 00:00 0                          [vvar]
7ffd4b1a5000-7ffd4b1a7000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Aborted (core dumped)
Comment 1 Martin Sebor 2017-03-16 22:28:41 UTC
The root cause is that the maybe_emit_sprintf_chk_warning function in builtins.c uses c_strlen() to compute the length of the string argument to the "%s" directive.  The function should instead leave the length computation to the check_sizes() function which has the smarts to figure out the (minimum) length even from a non-literal argument.

The following untested patch makes this change and allows the test case to be diagnosed.

@ -9734,33 +9793,26 @@ maybe_emit_sprintf_chk_warning (tree exp, enum built_in_function fcode)
   if (!init_target_chars ())
     return;
 
+  tree arg;
   /* If the format doesn't contain % args or %%, we know its size.  */
   if (strchr (fmt_str, target_percent) == 0)
-    len = build_int_cstu (size_type_node, strlen (fmt_str));
+    arg = build_int_cstu (size_type_node, strlen (fmt_str));
   /* If the format is "%s" and first ... argument is a string literal,
      we know it too.  */
   else if (fcode == BUILT_IN_SPRINTF_CHK
           && strcmp (fmt_str, target_percent_s) == 0)
     {
-      tree arg;
-
       if (nargs < 5)
        return;
       arg = CALL_EXPR_ARG (exp, 4);
       if (! POINTER_TYPE_P (TREE_TYPE (arg)))
        return;
-
-      len = c_strlen (arg, 1);
-      if (!len || ! tree_fits_uhwi_p (len))
-       return;
     }
   else
     return;
 
-  /* Add one for the terminating nul.  */
-  len = fold_build2 (PLUS_EXPR, TREE_TYPE (len), len, size_one_node);
   check_sizes (OPT_Wstringop_overflow_,
-              exp, /*size=*/NULL_TREE, /*maxlen=*/NULL_TREE, len, size);
+              exp, /*size=*/NULL_TREE, /*maxlen=*/NULL_TREE, arg, size);
 }
 
 /* Emit warning if a free is called with address of a variable.  */
Comment 2 Martin Sebor 2020-04-22 21:08:41 UTC
GCC 10 issues:

pr80074.c: In function ‘main’:
pr80074.c:7:53: warning: ‘%s’ directive writing 3 bytes into a region of size 1 [-Wformat-overflow=]
    7 |   int n = __builtin___sprintf_chk (d, 0, sizeof d, "%s", s);
      |                                                     ^~
pr80074.c:7:11: note: ‘__builtin___sprintf_chk’ output 4 bytes into a destination of size 1
    7 |   int n = __builtin___sprintf_chk (d, 0, sizeof d, "%s", s);
      |           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This was implemented in r240298:

commit 88d0c3f0a1448e71dcf49c2f34909ec8d7ce348f
Author: Martin Sebor <msebor@redhat.com>
Date:   Wed Sep 21 01:39:27 2016 +0000

    PR middle-end/49905 - Better sanity checking on sprintf src & dest to
    
    gcc/ChangeLog:
    
            PR middle-end/49905
            * Makefile.in (OBJS): Add gimple-ssa-sprintf.o.
            * config/linux.h (TARGET_PRINTF_POINTER_FORMAT): Redefine.
            * config/linux.c (gnu_libc_printf_pointer_format): New function.
            * config/sol2.h (TARGET_PRINTF_POINTER_FORMAT): Same.
            * config/sol2.c (solaris_printf_pointer_format): New function.
            * doc/invoke.texi (-Wformat-length, -fprintf-return-value): New
            options.
            * doc/tm.texi.in (TARGET_PRINTF_POINTER_FORMAT): Document.
            * doc/tm.texi: Regenerate.
            * gimple-fold.h (get_range_strlen): New function.
            (get_maxval_strlen): Declare existing function.
            * gimple-fold.c (get_range_strlen): Add arguments and compute both
            maximum and minimum.
             (get_range_strlen): Define overload.
            (get_maxval_strlen): Adjust.
            * gimple-ssa-sprintf.c: New file and pass.
            * passes.def (pass_sprintf_length): Add new pass.
            * targhooks.h (default_printf_pointer_format): Declare new function.
            (gnu_libc_printf_pointer_format): Same.
            (solaris_libc_printf_pointer_format): Same.
            * targhooks.c (default_printf_pointer_format): Define new function.
            * tree-pass.h (make_pass_sprintf_length): Declare new function.
            * print-tree.c: Increase buffer size.
    
    gcc/c-family/ChangeLog:
    
            PR middle-end/49905
            * c.opt: Add -Wformat-length and -fprintf-return-value.
    
    gcc/testsuite/ChangeLog:
    
            PR middle-end/49905
            * gcc.dg/builtin-stringop-chk-1.c: Adjust.
            * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: New test.
            * gcc.dg/tree-ssa/builtin-sprintf-warn-2.c: New test.
            * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: New test.
            * gcc.dg/tree-ssa/builtin-sprintf-warn-4.c: New test.
            * gcc.dg/tree-ssa/builtin-sprintf.c: New test.
            * gcc.dg/tree-ssa/builtin-sprintf-2.c: New test.
    
    From-SVN: r240298