Code: #include <string.h> char buf[2]; void test(const char* str) { strncpy(buf, "12345", sizeof("12345")); // 1 strncpy(buf, "12345", strlen("12345")); // 2 strncpy(buf, str, sizeof(str)); // 3 strncpy(buf, str, strlen(str)); // 4 } Compile command: gcc -c -o test.o -Wall -Wextra -O2 test.c -D_FORTIFY_SOURCE=2 When above code is compiled using gcc 4.8.5 on Linux RHEL 7 x86_64, gcc prints warning about line "3" (-Wsizeof-pointer-memaccess), plus there are two warnings for lines "1" and "3" detected by -D_FORTIFY_SOURCE=2. There are no warnings about buffer overflow in lines "2" and "4", where strlen of source is used instead of buffer size. What is interesting, gcc 5.4.0 from Cygwin x86_64 does not print warnings from -D_FORTIFY_SOURCE=2, only -Wsizeof-pointer-memaccess one. Please improve these checks, to detect cases when user will try to use sizeof or strlen of source string instead of target buffer size.
In this case this code will lead to buffer overflows, but in general case it often may work fine. However this is still error in code, and it would be good if gcc could detect and report it.
GCC 7 prints the warnings below (regardless of _FORTIFY_SOURCE). With earlier GCC versions the overflow warnings depend on the string functions being instrumented with GCC-specific Object Size Checking primitives. Unless this instrumentation is present on Cygwin the warnings won't trigger. It shouldn't be hard to diagnose call #4 in the simple cases where the last argument is a direct call to strlen(). Let me look into it for GCC 8. Thanks for the suggestion! $ gcc -O2 -S -Wall -Wextra -Wpedantic t.c In file included from /usr/include/string.h:630:0, from t.c:1: t.c: In function ‘test’: t.c:9:29: warning: argument to ‘sizeof’ in ‘__builtin_strncpy’ call is the same expression as the source; did you mean to provide an explicit length? [-Wsizeof-pointer-memaccess] strncpy(buf, str, sizeof(str)); // 3 ^ t.c:7:5: warning: ‘__builtin_memcpy’ writing 2 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=] strncpy(buf, "12345", sizeof("12345")); // 1 ^ t.c:8:5: warning: ‘__builtin_memcpy’ writing 5 bytes into a region of size 2 overflows the destination [-Wstringop-overflow=] strncpy(buf, "12345", strlen("12345")); // 2 ^ t.c:9:5: warning: ‘__builtin_strncpy’ writing 8 bytes into a region of size 2 overflows the destination [-Wstringop-overflow=] strncpy(buf, str, sizeof(str)); // 3
Testing a solution.
Patch: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00411.html
Redoing https://gcc.gnu.org/ml/gcc-bugs/2017-08/msg01672.html --- Comment #6 from Martin Sebor <msebor at gcc dot gnu.org> --- Author: msebor Date: Mon Aug 14 20:21:44 2017 New Revision: 251100 URL: https://gcc.gnu.org/viewcvs?rev=251100&root=gcc&view=rev Log: PR c/81117 - Improve buffer overflow checking in strncpy - part 2 gcc/ChangeLog: PR c/81117 * doc/extend.texi (attribute nonstring): Document new attribute. gcc/c-family/ChangeLog: PR c/81117 * c-attribs.c (c_common_attribute_table): Add nonstring entry. (handle_nonstring_attribute): New function. gcc/testsuite/ChangeLog: PR c/81117 * c-c++-common/attr-nonstring-1.c: New test. Modified: trunk/gcc/ChangeLog trunk/gcc/c-family/ChangeLog trunk/gcc/c-family/c-attribs.c trunk/gcc/doc/extend.texi trunk/gcc/testsuite/ChangeLog
Author: aldyh Date: Wed Sep 13 16:58:30 2017 New Revision: 252430 URL: https://gcc.gnu.org/viewcvs?rev=252430&root=gcc&view=rev Log: PR c/81117 - Improve buffer overflow checking in strncpy - part 1 gcc/ChangeLog: PR c/81117 * tree-diagnostic.c (default_tree_printer): Handle %G. * gimple-pretty-print.h (percent_G_format): Declare new function. * gimple-pretty-print.c (percent_G_format): Define. * tree-pretty-print.c (percent_K_format): Add argument. gcc/c/ChangeLog: PR c/81117 * c-objc-common.c (c_objc_common_init): Handle 'G'. gcc/c-family/ChangeLog: PR c/81117 * c-format.h (T89_G): New macro. * c-format.c (local_gcall_ptr_node): New variable. (init_dynamic_diag_info): Initialize it. gcc/cp/ChangeLog: PR c/81117 * error.c (cp_printer): Handle 'G'. gcc/testsuite/ChangeLog: PR c/81117 * gcc.dg/format/gcc_diag-10.c: Exercise %G. Modified: branches/range-gen2/gcc/ChangeLog branches/range-gen2/gcc/c-family/ChangeLog branches/range-gen2/gcc/c-family/c-format.c branches/range-gen2/gcc/c-family/c-format.h branches/range-gen2/gcc/c/ChangeLog branches/range-gen2/gcc/c/c-objc-common.c branches/range-gen2/gcc/cp/ChangeLog branches/range-gen2/gcc/cp/error.c branches/range-gen2/gcc/gimple-pretty-print.c branches/range-gen2/gcc/gimple-pretty-print.h branches/range-gen2/gcc/testsuite/ChangeLog branches/range-gen2/gcc/testsuite/gcc.dg/format/gcc_diag-10.c branches/range-gen2/gcc/tree-diagnostic.c branches/range-gen2/gcc/tree-pretty-print.c branches/range-gen2/gcc/tree-pretty-print.h
Author: aldyh Date: Wed Sep 13 16:58:44 2017 New Revision: 252431 URL: https://gcc.gnu.org/viewcvs?rev=252431&root=gcc&view=rev Log: PR c/81117 - Improve buffer overflow checking in strncpy - part 2 gcc/ChangeLog: PR c/81117 * doc/extend.texi (attribute nonstring): Document new attribute. gcc/c-family/ChangeLog: PR c/81117 * c-attribs.c (c_common_attribute_table): Add nonstring entry. (handle_nonstring_attribute): New function. gcc/testsuite/ChangeLog: PR c/81117 * c-c++-common/attr-nonstring-1.c: New test. Modified: branches/range-gen2/gcc/ChangeLog branches/range-gen2/gcc/c-family/ChangeLog branches/range-gen2/gcc/c-family/c-attribs.c branches/range-gen2/gcc/doc/extend.texi branches/range-gen2/gcc/testsuite/ChangeLog
Author: msebor Date: Fri Nov 10 16:35:26 2017 New Revision: 254630 URL: https://gcc.gnu.org/viewcvs?rev=254630&root=gcc&view=rev Log: PR c/81117 - Improve buffer overflow checking in strncpy gcc/ChangeLog: PR c/81117 * builtins.c (compute_objsize): Handle arrays that compute_builtin_object_size likes to fail for. Make extern. * builtins.h (compute_objsize): Declare. (check_strncpy_sizes): New function. (expand_builtin_strncpy): Call check_strncpy_sizes. * gimple-fold.c (gimple_fold_builtin_strncpy): Implement -Wstringop-truncation. (gimple_fold_builtin_strncat): Same. * gimple.c (gimple_build_call_from_tree): Set call location. * tree-ssa-strlen.c (strlen_to_stridx): New global variable. (maybe_diag_bound_equal_length, is_strlen_related_p): New functions. (handle_builtin_stxncpy, handle_builtin_strncat): Same. (handle_builtin_strlen): Use strlen_to_stridx. (strlen_optimize_stmt): Handle flavors of strncat, strncpy, and stpncpy. Use strlen_to_stridx. (pass_strlen::execute): Release strlen_to_stridx. * doc/invoke.texi (-Wsizeof-pointer-memaccess): Document enhancement. (-Wstringop-truncation): Document new option. gcc/ada/ChangeLog: PR c/81117 * ada/adadecode.c (__gnat_decode): Use memcpy instead of strncpy. * ada/argv.c (__gnat_fill_arg, __gnat_fill_env): Same. gcc/c-family/ChangeLog: PR c/81117 * c-common.c (catenate_strings): Use memcpy instead of strncpy. * c-warn.c (sizeof_pointer_memaccess_warning): Handle arrays. * c.opt (-Wstringop-truncation): New option. gcc/fortran/ChangeLog: PR c/81117 * gcc/fortran/decl.c (build_sym): Use strcpy instead of strncpy. gcc/objc/ChangeLog: PR c/81117 * objc-encoding.c (encode_type): Use memcpy instead of strncpy. gcc/testsuite/ChangeLog: PR c/81117 * c-c++-common/Wsizeof-pointer-memaccess3.c: New test. * c-c++-common/Wstringop-overflow.c: Same. * c-c++-common/Wstringop-truncation.c: Same. * c-c++-common/Wsizeof-pointer-memaccess2.c: Adjust. * c-c++-common/attr-nonstring-2.c: New test. * g++.dg/torture/Wsizeof-pointer-memaccess1.C: Adjust. * g++.dg/torture/Wsizeof-pointer-memaccess2.C: Same. * gcc.dg/torture/pr63554.c: Same. * gcc.dg/Walloca-1.c: Disable macro tracking. Added: trunk/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess3.c trunk/gcc/testsuite/c-c++-common/Wstringop-overflow.c trunk/gcc/testsuite/c-c++-common/Wstringop-truncation.c trunk/gcc/testsuite/c-c++-common/attr-nonstring-1.c trunk/gcc/testsuite/c-c++-common/attr-nonstring-2.c Modified: trunk/gcc/ChangeLog trunk/gcc/ada/ChangeLog trunk/gcc/ada/adadecode.c trunk/gcc/ada/argv.c trunk/gcc/builtins.c trunk/gcc/builtins.h trunk/gcc/c-family/ChangeLog trunk/gcc/c-family/c-common.c trunk/gcc/c-family/c-warn.c trunk/gcc/c-family/c.opt trunk/gcc/doc/invoke.texi trunk/gcc/fortran/ChangeLog trunk/gcc/fortran/decl.c trunk/gcc/gimple-fold.c trunk/gcc/gimple.c trunk/gcc/objc/ChangeLog trunk/gcc/objc/objc-encoding.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c trunk/gcc/testsuite/g++.dg/torture/Wsizeof-pointer-memaccess1.C trunk/gcc/testsuite/g++.dg/torture/Wsizeof-pointer-memaccess2.C trunk/gcc/testsuite/gcc.dg/Walloca-1.c trunk/gcc/testsuite/gcc.dg/builtin-stpncpy.c trunk/gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c trunk/gcc/testsuite/gcc.dg/torture/pr63554.c trunk/gcc/tree-ssa-strlen.c
The enhancement has been committed in r254630. With the slightly modified test case GCC 8.0 produces the warnings below. Unfortunately, including <string.h> instead of explicitly declaring strncpy may suppress a number of the warnings, including the one for the last call, when strncpy is defined as a macro in one of the system headers (as in Glibc 2.24 and prior). I've raised bug 82944 for the system header problem and so I'm resolving this request as fixed. $ cat pr81117.c && gcc -O2 -S -Wall -Wextra pr81117.c extern __SIZE_TYPE__ strlen (const char*); extern char* strncpy (char*, const char*, __SIZE_TYPE__); char buf[2]; void test (const char* str) { strncpy (buf, "12345", sizeof ("12345")); // 1 strncpy (buf, "12345", strlen ("12345")); // 2 strncpy (buf, str, sizeof (str)); // 3 strncpy (buf, str, strlen (str)); // 4 } pr81117.c: In function ‘test’: pr81117.c:8:35: warning: argument to ‘sizeof’ in ‘strncpy’ call is the same expression as the source; did you mean to use the size of the destination? [-Wsizeof-pointer-memaccess] strncpy (buf, "12345", sizeof ("12345")); // 1 ^ pr81117.c:10:31: warning: argument to ‘sizeof’ in ‘strncpy’ call is the same expression as the source; did you mean to provide an explicit length? [-Wsizeof-pointer-memaccess] strncpy (buf, str, sizeof (str)); // 3 ^ pr81117.c:9:5: warning: ‘strncpy’ output truncated before terminating nul copying 5 bytes from a string of the same length [-Wstringop-truncation] strncpy (buf, "12345", strlen ("12345")); // 2 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pr81117.c:8:5: warning: array subscript is above array bounds [-Warray-bounds] strncpy (buf, "12345", sizeof ("12345")); // 1 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pr81117.c:11:5: warning: ‘strncpy’ output truncated before terminating nul copying as many bytes from a string as its length [-Wstringop-truncation] strncpy (buf, str, strlen (str)); // 4 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pr81117.c:8:5: warning: ‘__builtin_memcpy’ writing 2 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=] strncpy (buf, "12345", sizeof ("12345")); // 1 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pr81117.c:9:5: warning: ‘__builtin_memcpy’ writing 5 bytes into a region of size 2 overflows the destination [-Wstringop-overflow=] strncpy (buf, "12345", strlen ("12345")); // 2 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ pr81117.c:10:5: warning: ‘strncpy’ writing 8 bytes into a region of size 2 overflows the destination [-Wstringop-overflow=] strncpy (buf, str, sizeof (str)); // 3 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Author: msebor Date: Fri Nov 10 22:48:43 2017 New Revision: 254641 URL: https://gcc.gnu.org/viewcvs?rev=254641&root=gcc&view=rev Log: gcc/ChangeLog: PR c/81117 * config/darwin-c.c (framework_construct_pathname): Replace strncpy with memcpy. (find_subframework_file): Same. Modified: trunk/gcc/ChangeLog trunk/gcc/config/darwin-c.c
Author: msebor Date: Sat Nov 11 18:04:21 2017 New Revision: 254659 URL: https://gcc.gnu.org/viewcvs?rev=254659&root=gcc&view=rev Log: gcc/ChangeLog: PR c/81117 * doc/extend.texi (attribute nonstring): Remove spurious argument. Modified: trunk/gcc/ChangeLog trunk/gcc/doc/extend.texi
char *strncpy(char *dest, const char *src, size_t n); void foo(char* p) { strncpy(p, "1", 1); p[1] = 0; } with gcc8/r254663 is this expected? $ gcc -c -Wall x.c x.c: In function ‘foo’: x.c:4:5: warning: ‘strncpy’ output truncated before terminating nul copying 1 byte from a string of the same length [-Wstringop-truncation] strncpy(p, "1", 1); ^~~~~~~~~~~~~~~~~~
Sounds like -Wno-stringop-overflow does not propagate into LTO build. I'll try make a small testcase
(In reply to Dmitry G. Dyachenko from comment #12) I'm afraid the warning in the constant string case is unavoidable. The call is folded at a point where the checker doesn't have access to the subsequent statement. At the same time, it can be viewed as a feature since the code would be more clearly written simply as strcpy(p, "1").
(In reply to Dmitry G. Dyachenko from comment #12) LTO doesn't interact with these warnings very well. pr71907 and pr79062 track a couple of the problems I know about. If you find a different issue please do open a new bug.
(In reply to Martin Sebor from comment #15) Thank you. Nice warnings!
(In reply to Martin Sebor from comment #14) > (In reply to Dmitry G. Dyachenko from comment #12) > > I'm afraid the warning in the constant string case is unavoidable. The call > is folded at a point where the checker doesn't have access to the subsequent > statement. At the same time, it can be viewed as a feature since the code > would be more clearly written simply as strcpy(p, "1"). This can be resolved in this way. However strcpy is not recommended because it does not check buffer size and can cause buffer overflow (however in this particular case it would be safe to use). It would be good to use strlcpy here, however it is not supported by gcc/glibc now. Maybe these new warnings will help with adding them to gcc/glibc, I saw that many people in the past requested this but without luck.
Hi, I'm seeing random results on: c-c++-common/Wstringop-truncation.c -Wc++-compat (test for warnings, line 210) c-c++-common/Wstringop-truncation.c -Wc++-compat (test for warnings, line 211) on aarch64/arm targets if that matters. By random, I mean sometimes these two tests pass, sometimes they fail. May it mean that the patch (r254630) has an undefined behavior code path?
(In reply to Christophe Lyon from comment #18) Those are most likely due to pr82977 that Jakub fixed just yesterday. If the problems persist please open a new bug (running the test through valgrind might help pinpoint the root cause).
this breaks glibc-2.26: ../sysdeps/unix/sysv/linux/if_index.c: In function '__if_nametoindex': ../sysdeps/unix/sysv/linux/if_index.c:46:3: error: 'strncpy' specified bound 16 equals destination size [-Werror=stringop-truncation] strncpy (ifr.ifr_name, ifname, sizeof (ifr.ifr_name)); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ cc1: all warnings being treated as errors make[2]: *** [/home/ed/gnu/glibc-build/inet/if_index.o] Error 1 but in this case the code is correct: struct ifreq ifr; [...] strncpy (ifr.ifr_name, ifname, sizeof (ifr.ifr_name)); if (__ioctl (fd, SIOCGIFINDEX, &ifr) < 0) Because ifr_name does not need zero-termination. And the following would not be correct, but does not warn: strncpy (ifr.ifr_name, ifname, sizeof (ifr.ifr_name)-1);
I believe the bug you are pointing out was reported in https://sourceware.org/bugzilla/show_bug.cgi?id=22442 and fixed in Glibc 2.27. Please see the discussion at https://sourceware.org/ml/libc-alpha/2017-11/msg00336.html for the background.
(In reply to Martin Sebor from comment #21) > I believe the bug you are pointing out was reported in > https://sourceware.org/bugzilla/show_bug.cgi?id=22442 and fixed in Glibc > 2.27. Please see the discussion at > https://sourceware.org/ml/libc-alpha/2017-11/msg00336.html for the > background. Ok, I see, thanks. Maybe an info in this warning message pointing out the other possibility (using __attribute__ ((__nonstring__))) to resolve the other use case would be helpful ?
I agree that mentioning the attribute in the documentation of the -Wstringop-truncation option would be helpful. Let me submit a patch with that change. I considered having GCC issue a note suggesting the attribute but decided that using it was sufficiently rare and specialized that it would be a distraction in most situations. Of the warnings the option found in GCC, GDB, and Glibc, using the attribute was a solution in just three instances so far, all in Glibc: https://sourceware.org/git/gitweb.cgi?p=glibc.git;a=commit;h=7532837d7b03b3ca5b9a63d77a5bd81dd23f3d9c Using attribute nonstring inappropriately could also lead to more warnings because it triggers its own checks that aren't suitable for nul-terminated strings -- see pr82945.