handle sprintf(d, "%s", ...) in gimple-ssa-sprintf.c

Message ID	64b29bd5-d59e-8268-5042-285912738ae4@gmail.com
State	New
Headers	show Return-Path: <gcc-patches-return-451871-incoming=patchwork.ozlabs.org@gcc.gnu.org> DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :subject:to:message-id:date:mime-version:content-type; q=dns; s= default; b=N7m6GmG9VChyaXicTdKx0/Z4UU3xpP0ZSuEeKDjJAFa+Gln/fr9fa 8ixbYzpS+L9KgbAYFqquV90eGP1y/NbUJmggRIfG9CFB6dMwI/tdWctusrgtDpdl /crr5LLZtOGQz5OzRJ/2TQJNkgIA14IrB9zWblZ3E9KHoVqbWdXbM4= Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk Sender: gcc-patches-owner@gcc.gnu.org From: Martin Sebor <msebor@gmail.com> Subject: [PATCH] handle sprintf(d, "%s", ...) in gimple-ssa-sprintf.c To: Gcc Patch List <gcc-patches@gcc.gnu.org> Message-ID: <64b29bd5-d59e-8268-5042-285912738ae4@gmail.com> Date: Fri, 21 Apr 2017 15:33:14 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------391421AD7B6143845C0995B8"

On Fri, Apr 28, 2017 at 6:35 PM, Jeff Law <law@redhat.com> wrote: > On 04/25/2017 09:55 PM, Martin Sebor wrote: >> >> On 04/25/2017 04:05 PM, Jeff Law wrote: >>> >>> On 04/21/2017 03:33 PM, Martin Sebor wrote: >>>> >>>> Bug 77671 - missing -Wformat-overflow warning on sprintf overflow >>>> with "%s", is caused by gimple-fold.c transforming s{,n}printf >>>> calls with a plain "%s" format string into strcpy regardless of >>>> whether they are valid well before the -Wformtat-overflow warning >>>> has had a chance to validate them. >>>> >>>> The attached patch moves the transformation from gimple-fold.c >>>> into the gimple-ssa-sprintf.c pass, thus allowing the calls to >>>> be validated. Only valid calls (i.e., those that do not overflow >>>> the destination) are transformed. >>>> >>>> Martin >>>> >>>> gcc-77671.diff >>>> >>>> >>>> commit 2cd98763984ffb93606ee96ad658733b4c95c1e4 >>>> Author: Martin Sebor<msebor@redhat.com> >>>> Date: Wed Apr 12 18:36:26 2017 -0600 >>>> >>>> PR middle-end/77671 - missing -Wformat-overflow warning on >>>> sprintf overflow >>>> with %s >>>> gcc/ChangeLog: >>>> PR middle-end/77671 >>>> * gimple-fold.c (gimple_fold_builtin_sprintf): Make extern. >>>> (gimple_fold_builtin_snprintf): Same. >>>> * gimple-fold.h (gimple_fold_builtin_sprintf): Declare. >>>> (gimple_fold_builtin_snprintf): Same. >>>> * gimple-ssa-sprintf.c (get_format_string): Correct the >>>> detection >>>> of character types. >>>> (is_call_safe): New function. >>>> (try_substitute_return_value): Call it. >>>> (try_simplify_call): New function. >>>> (pass_sprintf_length::handle_gimple_call): Call it. >>>> gcc/testsuite/ChangeLog: >>>> PR middle-end/77671 >>>> * gcc.dg/tree-ssa/builtin-sprintf-7.c: New test. >>>> * gcc.dg/tree-ssa/builtin-sprintf-8.c: New test. >>>> * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Adjust. >>>> * gcc.dg/tree-ssa/builtin-sprintf-warn-2.c: Adjust. >>>> * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Adjust. >>> >>> I assume this went through the usual regression testing cycle? I'm a >>> bit surprised nothing failed due to moving the transformation later in >>> the pipeline. >> >> >> It did. There was one regression I neglected to mention. A test >> exercising -fexec-charset (bug 20110) fails because the sprintf >> pass assumes the execution character set is the same as the host >> character set. With -fexec-charset set to EBCDIC it gets just >> as confused as the -Wformat warning does (this is subject of >> the still unresolved bug 20110). I've opened bug 80523 for >> the new problem and I'm testing a patch that handles it. >> >> >>>> diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c >>>> index 2474391..07d6897 100644 >>>> --- a/gcc/gimple-ssa-sprintf.c >>>> +++ b/gcc/gimple-ssa-sprintf.c >>>> @@ -373,7 +373,16 @@ get_format_string (tree format, location_t *ploc) >>>> if (TREE_CODE (format) != STRING_CST) >>>> return NULL; >>>> - if (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (format))) != >>>> char_type_node) >>>> + tree type = TREE_TYPE (format); >>>> + if (TREE_CODE (type) == ARRAY_TYPE) >>>> + type = TREE_TYPE (type); >>>> + >>>> + /* Can't just test that: >>>> + TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (format))) != >>>> char_type_node >>>> + See bug 79062. */ >>>> + if (TREE_CODE (type) != INTEGER_TYPE >>>> + || TYPE_MODE (type) != TYPE_MODE (char_type_node) >>>> + || TYPE_PRECISION (type) != TYPE_PRECISION (char_type_node)) >>>> { >>>> /* Wide format string. */ >>>> return NULL; >>> >>> In the referenced BZ Richi mentions how fold-const.c checks for this >>> case and also hints that you might want t look at tree-ssa-strlen as >>> well. That seems wise. It also seems wise to factor that check and use >>> it from all the identified locations. I don't like that this uses a >>> different check than what's in fold-const.c. >> >> >> I think what I did comes from tree-ssa-strlen.c (Richi's other >> suggestion in bug 79062). In either case I don't fully understand >> why the existing code doesn't work. >> >>> It's also not clear if this change is a requirement to address 77671 or >>> not. If so ISTM that we fix 79062 first, then 77671. If not we can fix >>> them independently. In both cases the fix for 79062 can be broken out >>> into its own patch. >> >> >> I suppose that makes sense. The hunk above doesn't fully fix >> 79062. It just lets the sprintf return value optimization take >> place with -flto. >> >>>> @@ -3278,31 +3287,21 @@ get_destination_size (tree dest) >>>> return HOST_WIDE_INT_M1U; >>>> } >>>> -/* Given a suitable result RES of a call to a formatted output >>>> function >>>> - described by INFO, substitute the result for the return value of >>>> - the call. The result is suitable if the number of bytes it >>>> represents >>>> - is known and exact. A result that isn't suitable for substitution >>>> may >>>> - have its range set to the range of return values, if that is known. >>>> - Return true if the call is removed and gsi_next should not be >>>> performed >>>> - in the caller. */ >>>> - >>>> static bool >>>> -try_substitute_return_value (gimple_stmt_iterator *gsi, >>>> - const pass_sprintf_length::call_info &info, >>>> - const format_result &res) >>>> +is_call_safe (const pass_sprintf_length::call_info &info, >>>> + const format_result &res, bool under4k, >>>> + unsigned HOST_WIDE_INT retval[2]) >>> >>> You need a function comment for is_call_safe. >>> >>> >>> >>>> @@ -3423,6 +3458,30 @@ try_substitute_return_value >>>> (gimple_stmt_iterator *gsi, >>>> return removed; >>>> } >>>> +static bool >>>> +try_simplify_call (gimple_stmt_iterator *gsi, >>>> + const pass_sprintf_length::call_info &info, >>>> + const format_result &res) >>> >>> Needs a function comment. >>> >>> >>> >>> So there's the minor nits to add function comments. The big question >>> is the stuff for 79062 and a confirmation that this went through a full >>> regression test cycle. >> >> >> I think I may have included the (partial) the fix for 79062 to >> get some tests to pass but I'm not 100% sure. >> >> Let me first submit the fix for the -fexec-charset limitation >> (bug 80523), see if I can separate out the partial fix for 79062, >> and then resubmit this patch. > > OK. I think 80523 is pretty close. I mentioned one implementation detail > (initialize translation table once per file rather than once per function) > and one question about what happens for long translated strings. I suspect > you'll have that one good-to-go shortly. > >> >> FWIW, my fix for bug 79062 is only partial (it gets the pass >> to run but the warnings are still not issued). I don't quite >> understand what prevents the warning flag(s) from getting set >> when -flto is used. This seems to be a bigger problem than >> just the sprintf pass not doing something just right. > > I've never dug deeply in the LTO stuff, but I believe we stream the compiler > flags, so it could be something there. We do. > Alternately you might be running into a case where in LTO mode we recreate > base types. Look for a type equality tester that goes beyond just testing > pointer equality. > > ie, in LTO I think we'll create a type based on the streamed data, but I > also think we'll create various basic types. Thus in LTO mode pointer > equality may not be sufficient. We make sure that for most basic types we end up re-using them where possible. char_type_node is an example where that generally doesn't work because it's value depends on a command-line flag. Richard. > jeff

commit 2cd98763984ffb93606ee96ad658733b4c95c1e4 Author: Martin Sebor <msebor@redhat.com> Date: Wed Apr 12 18:36:26 2017 -0600 PR middle-end/77671 - missing -Wformat-overflow warning on sprintf overflow with %s gcc/ChangeLog: PR middle-end/77671 * gimple-fold.c (gimple_fold_builtin_sprintf): Make extern. (gimple_fold_builtin_snprintf): Same. * gimple-fold.h (gimple_fold_builtin_sprintf): Declare. (gimple_fold_builtin_snprintf): Same. * gimple-ssa-sprintf.c (get_format_string): Correct the detection of character types. (is_call_safe): New function. (try_substitute_return_value): Call it. (try_simplify_call): New function. (pass_sprintf_length::handle_gimple_call): Call it. gcc/testsuite/ChangeLog: PR middle-end/77671 * gcc.dg/tree-ssa/builtin-sprintf-7.c: New test. * gcc.dg/tree-ssa/builtin-sprintf-8.c: New test. * gcc.dg/tree-ssa/builtin-sprintf-warn-1.c: Adjust. * gcc.dg/tree-ssa/builtin-sprintf-warn-2.c: Adjust. * gcc.dg/tree-ssa/builtin-sprintf-warn-3.c: Adjust. diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c index 9fd45d1..aa53c33 100644 --- a/gcc/gimple-fold.c +++ b/gcc/gimple-fold.c @@ -2670,11 +2670,9 @@ gimple_fold_builtin_sprintf_chk (gimple_stmt_iterator *gsi, ORIG may be null if this is a 2-argument call. We don't attempt to simplify calls with more than 3 arguments. - Return NULL_TREE if no simplification was possible, otherwise return the - simplified form of the call as a tree. If IGNORED is true, it means that - the caller does not use the returned value of the function. */ + Return true if simplification was possible, otherwise false. */ -static bool +bool gimple_fold_builtin_sprintf (gimple_stmt_iterator *gsi) { gimple *stmt = gsi_stmt (*gsi); @@ -2795,11 +2793,9 @@ gimple_fold_builtin_sprintf (gimple_stmt_iterator *gsi) FMT, and ORIG. ORIG may be null if this is a 3-argument call. We don't attempt to simplify calls with more than 4 arguments. - Return NULL_TREE if no simplification was possible, otherwise return the - simplified form of the call as a tree. If IGNORED is true, it means that - the caller does not use the returned value of the function. */ + Return true if simplification was possible, otherwise false. */ -static bool +bool gimple_fold_builtin_snprintf (gimple_stmt_iterator *gsi) { gcall *stmt = as_a <gcall *> (gsi_stmt (*gsi)); @@ -3362,10 +3358,7 @@ gimple_fold_builtin (gimple_stmt_iterator *gsi) case BUILT_IN_SNPRINTF_CHK: case BUILT_IN_VSNPRINTF_CHK: return gimple_fold_builtin_snprintf_chk (gsi, fcode); - case BUILT_IN_SNPRINTF: - return gimple_fold_builtin_snprintf (gsi); - case BUILT_IN_SPRINTF: - return gimple_fold_builtin_sprintf (gsi); + case BUILT_IN_FPRINTF: case BUILT_IN_FPRINTF_UNLOCKED: case BUILT_IN_VFPRINTF: diff --git a/gcc/gimple-fold.h b/gcc/gimple-fold.h index e4931a1..85c52d3 100644 --- a/gcc/gimple-fold.h +++ b/gcc/gimple-fold.h @@ -53,6 +53,8 @@ extern tree gimple_get_virt_method_for_vtable (HOST_WIDE_INT, tree, unsigned HOST_WIDE_INT, bool *can_refer = NULL); extern tree gimple_fold_indirect_ref (tree); +extern bool gimple_fold_builtin_sprintf (gimple_stmt_iterator *); +extern bool gimple_fold_builtin_snprintf (gimple_stmt_iterator *); extern bool arith_code_with_undefined_signed_overflow (tree_code); extern gimple_seq rewrite_to_defined_overflow (gimple *); diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c index 2474391..07d6897 100644 --- a/gcc/gimple-ssa-sprintf.c +++ b/gcc/gimple-ssa-sprintf.c @@ -373,7 +373,16 @@ get_format_string (tree format, location_t *ploc) if (TREE_CODE (format) != STRING_CST) return NULL; - if (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (format))) != char_type_node) + tree type = TREE_TYPE (format); + if (TREE_CODE (type) == ARRAY_TYPE) + type = TREE_TYPE (type); + + /* Can't just test that: + TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (format))) != char_type_node + See bug 79062. */ + if (TREE_CODE (type) != INTEGER_TYPE + || TYPE_MODE (type) != TYPE_MODE (char_type_node) + || TYPE_PRECISION (type) != TYPE_PRECISION (char_type_node)) { /* Wide format string. */ return NULL; @@ -3278,31 +3287,21 @@ get_destination_size (tree dest) return HOST_WIDE_INT_M1U; } -/* Given a suitable result RES of a call to a formatted output function - described by INFO, substitute the result for the return value of - the call. The result is suitable if the number of bytes it represents - is known and exact. A result that isn't suitable for substitution may - have its range set to the range of return values, if that is known. - Return true if the call is removed and gsi_next should not be performed - in the caller. */ - static bool -try_substitute_return_value (gimple_stmt_iterator *gsi, - const pass_sprintf_length::call_info &info, - const format_result &res) +is_call_safe (const pass_sprintf_length::call_info &info, + const format_result &res, bool under4k, + unsigned HOST_WIDE_INT retval[2]) { - tree lhs = gimple_get_lhs (info.callstmt); - - /* Set to true when the entire call has been removed. */ - bool removed = false; + if (under4k && !res.under4k) + return false; /* The minimum return value. */ - unsigned HOST_WIDE_INT minretval = res.range.min; + retval[0] = res.range.min; /* The maximum return value is in most cases bounded by RES.RANGE.MAX but in cases involving multibyte characters could be as large as RES.RANGE.UNLIKELY. */ - unsigned HOST_WIDE_INT maxretval + retval[1] = res.range.unlikely < res.range.max ? res.range.max : res.range.unlikely; /* Adjust the number of bytes which includes the terminating nul @@ -3311,10 +3310,10 @@ try_substitute_return_value (gimple_stmt_iterator *gsi, a valid range before the adjustment below is [0, INT_MAX + 1] (the functions only return negative values on error or undefined behavior). */ - if (minretval <= target_int_max () + 1) - --minretval; - if (maxretval <= target_int_max () + 1) - --maxretval; + if (retval[0] <= target_int_max () + 1) + --retval[0]; + if (retval[1] <= target_int_max () + 1) + --retval[1]; /* Avoid the return value optimization when the behavior of the call is undefined either because any directive may have produced 4K or @@ -3322,16 +3321,52 @@ try_substitute_return_value (gimple_stmt_iterator *gsi, the output overflows the destination object (but leave it enabled when the function is bounded because then the behavior is well- defined). */ - if (res.under4k - && minretval == maxretval - && (info.bounded || minretval < info.objsize) - && minretval <= target_int_max () + if (retval[0] == retval[1] + && (info.bounded || retval[0] < info.objsize) + && retval[0] <= target_int_max ()) + return true; + + if ((info.bounded || retval[1] < info.objsize) + && (retval[0] < target_int_max () + && retval[1] < target_int_max ())) + return true; + + if (!under4k && (info.bounded || retval[0] < info.objsize)) + return true; + + return false; +} + +/* Given a suitable result RES of a call to a formatted output function + described by INFO, substitute the result for the return value of + the call. The result is suitable if the number of bytes it represents + is known and exact. A result that isn't suitable for substitution may + have its range set to the range of return values, if that is known. + Return true if the call is removed and gsi_next should not be performed + in the caller. */ + +static bool +try_substitute_return_value (gimple_stmt_iterator *gsi, + const pass_sprintf_length::call_info &info, + const format_result &res) +{ + tree lhs = gimple_get_lhs (info.callstmt); + + /* Set to true when the entire call has been removed. */ + bool removed = false; + + /* The minimum and maximum return value. */ + unsigned HOST_WIDE_INT retval[2]; + bool safe = is_call_safe (info, res, true, retval); + + if (safe + && retval[0] == retval[1] /* Not prepared to handle possibly throwing calls here; they shouldn't appear in non-artificial testcases, except when the __*_chk routines are badly declared. */ && !stmt_ends_bb_p (info.callstmt)) { - tree cst = build_int_cst (integer_type_node, minretval); + tree cst = build_int_cst (integer_type_node, retval[0]); if (lhs == NULL_TREE && info.nowrite) @@ -3379,18 +3414,18 @@ try_substitute_return_value (gimple_stmt_iterator *gsi, { bool setrange = false; - if ((info.bounded || maxretval < info.objsize) - && res.under4k - && (minretval < target_int_max () - && maxretval < target_int_max ())) + if (safe + && (info.bounded || retval[1] < info.objsize) + && (retval[0] < target_int_max () + && retval[1] < target_int_max ())) { /* If the result is in a valid range bounded by the size of the destination set it so that it can be used for subsequent optimizations. */ int prec = TYPE_PRECISION (integer_type_node); - wide_int min = wi::shwi (minretval, prec); - wide_int max = wi::shwi (maxretval, prec); + wide_int min = wi::shwi (retval[0], prec); + wide_int max = wi::shwi (retval[1], prec); set_range_info (lhs, VR_RANGE, min, max); setrange = true; @@ -3399,21 +3434,21 @@ try_substitute_return_value (gimple_stmt_iterator *gsi, if (dump_file) { const char *inbounds - = (minretval < info.objsize - ? (maxretval < info.objsize + = (retval[0] < info.objsize + ? (retval[1] < info.objsize ? "in" : "potentially out-of") : "out-of"); const char *what = setrange ? "Setting" : "Discarding"; - if (minretval != maxretval) + if (retval[0] != retval[1]) fprintf (dump_file, " %s %s-bounds return value range [%llu, %llu].\n", what, inbounds, - (unsigned long long)minretval, - (unsigned long long)maxretval); + (unsigned long long)retval[0], + (unsigned long long)retval[1]); else fprintf (dump_file, " %s %s-bounds return value %llu.\n", - what, inbounds, (unsigned long long)minretval); + what, inbounds, (unsigned long long)retval[0]); } } @@ -3423,6 +3458,30 @@ try_substitute_return_value (gimple_stmt_iterator *gsi, return removed; } +static bool +try_simplify_call (gimple_stmt_iterator *gsi, + const pass_sprintf_length::call_info &info, + const format_result &res) +{ + unsigned HOST_WIDE_INT dummy[2]; + if (!is_call_safe (info, res, info.retval_used (), dummy)) + return false; + + switch (info.fncode) + { + case BUILT_IN_SNPRINTF: + return gimple_fold_builtin_snprintf (gsi); + + case BUILT_IN_SPRINTF: + return gimple_fold_builtin_sprintf (gsi); + + default: + ; + } + + return false; +} + /* Determine if a GIMPLE CALL is to one of the sprintf-like built-in functions and if so, handle it. Return true if the call is removed and gsi_next should not be performed in the caller. */ @@ -3674,13 +3733,23 @@ pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi) attempt to substitute the computed result for the return value of the call. Avoid this optimization when -frounding-math is in effect and the format string contains a floating point directive. */ - if (success - && optimize > 0 - && flag_printf_return_value - && (!flag_rounding_math || !res.floating)) - return try_substitute_return_value (gsi, info, res); + bool call_removed = false; + if (success && optimize > 0) + { + /* Save a copy of the iterator pointing at the call. The iterator + may change to point past the call in try_substitute_return_value + but the original value is needed in try_simplify_call. */ + gimple_stmt_iterator gsi_call = *gsi; - return false; + if (flag_printf_return_value + && (!flag_rounding_math || !res.floating)) + call_removed = try_substitute_return_value (gsi, info, res); + + if (!call_removed) + try_simplify_call (&gsi_call, info, res); + } + + return call_removed; } /* Execute the pass for function FUN. */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-7.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-7.c new file mode 100644 index 0000000..29954aa --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-7.c @@ -0,0 +1,99 @@ +/* PR tree-optimization/77671 - missing -Wformat-overflow warning + on sprintf overflow with "%s" + { dg-compile } + { dg-options "-O2 -Wformat -Wno-format-zero-length -fdump-tree-optimized" } */ + +void sink (char*); + +extern char buffer[]; + +/* String exactly 4100 characters long (plus the terminating NUL). */ +extern const char s4100[4101]; + +void test_sprintf (const char *s) +{ +#define IGN(...) __builtin_sprintf (buffer, __VA_ARGS__); sink (buffer) + + /* Each of the following calls is expected to be transformed into + one of memcpy or strcpy. */ + IGN (""); + IGN ("a"); + IGN ("ab"); + /* FIXME: Transform to strcpy/memcpy. */ + /* IGN (s4100 + 5); */ + + IGN ("%s", ""); + IGN ("%s", "a"); + IGN ("%s", "ab"); + + IGN ("%s", s4100 + 5); + + /* FIXME: This can be transformed into strcpy. */ + /* IGN (s); */ + IGN ("%s", s); +} + + +void test_snprintf (void) +{ +#undef IGN +#define IGN(N, ...) __builtin_snprintf (buffer, N, __VA_ARGS__); sink (buffer) + + /* Each of the following calls is expected to be transformed into + one of memcpy or strcpy. */ + IGN (1, ""); + IGN (2, "1"); + IGN (8, "1234567"); + + /* FIXME: Transform to strcpy/memcpy. */ + /* IGN (4096, s4100 + 5); */ + + IGN (1, "%s", ""); + IGN (2, "%s", "1"); + IGN (8, "%s", "1234567"); + + IGN (4096, "%s", s4100 + 5); +} + +#if 0 /* FIXME: Implement vs{,n}printf optimization. */ + +void test_vsprintf (__builtin_va_list va) +{ +#undef IGN +#define IGN(fmt) __builtin_vsprintf (buffer, fmt, va); sink (buffer) + + /* Each of the following calls is expected to be transformed into + one of memcpy or strcpy. */ + IGN (""); + IGN ("a"); + IGN ("ab"); + IGN (s4100 + 5); + + IGN ("%s"); +} + +void test_vsnprintf (__builtin_va_list va) +{ +#undef IGN +#define IGN(N, fmt) __builtin_vsnprintf (buffer, N, fmt, va); sink (buffer) + + /* Each of the following calls is expected to be transformed into + one of memcpy or strcpy. */ + IGN ( 1, ""); + IGN ( 2, "1"); + IGN ( 8, "1234567"); + IGN (4096, s4100 + 5); +} + +#endif + +/* { dg-final { scan-tree-dump-not "builtin_sprintf" "optimized" } } + { dg-final { scan-tree-dump-not "builtin_snprintf" "optimized" } } + { dg-final { scan-tree-dump-not "builtin_vsprintf" "optimized" } } + { dg-final { scan-tree-dump-not "builtin_vsnprintf" "optimized" } } */ + +#define S10 "0123456789" +#define S100 S10 S10 S10 S10 S10 S10 S10 S10 S10 S10 +#define S1000 S100 S100 S100 S100 S100 S100 S100 S100 S100 S100 + +const char s4100[4101] = S1000 S1000 S1000 S1000 S100; diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-8.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-8.c new file mode 100644 index 0000000..2d38d12 --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-8.c @@ -0,0 +1,104 @@ +/* PR tree-optimization/77671 - missing -Wformat-overflow warning + on sprintf overflow with "%s" + + Negative test verifying that sprintf family calls that must not + be transformed into calls to other functions (such as memcpy) + are preserved. + + { dg-compile } + { dg-options "-O2 -Wformat -Wno-format-truncation -Wno-format-zero-length -fdump-tree-optimized" } */ + +void sink (char*, ...); + +extern char buffer[]; + +/* String exactly 4100 characters long (plus the terminating NUL). */ +extern const char s4100[4101]; + +void test_sprintf (const char *s) +{ + /* Macros to test the function call while eignoring and using + the return value, respectively. */ +#define IGN(...) __builtin_sprintf (buffer, __VA_ARGS__), sink (buffer) +#define USE(...) sink (buffer, __builtin_sprintf (buffer, __VA_ARGS__)) + + /* All of the following calls to sprintf must be emitted (and not + transformed into memcpy, strcpy, or similar). */ + + /* Verify that a sprintf call with output in excess of the maximum + of 4095 bytes is not transformed into memcpy/strcpy when its + return value is used (the call may fail with EOVERFLOW but + the error is only detectable when the function's negative return + value indicates errno is valid ). */ + USE (s4100); + + USE ("%s", s4100); + + /* Same as above but with string of unknown length (which could + be in excess of 4K long). */ + USE (s); + USE ("%s", s); +} + + +void test_snprintf (void) +{ +#undef IGN +#define IGN(N, ...) __builtin_snprintf (buffer, N, __VA_ARGS__); sink (buffer) + + /* All of the following calls to sprintf must be emitted (and not + transformed into memcpy, strcpy, or similar). */ + + /* Truncated output could be optimized into strncpy (not done yet). */ + IGN (1, "123"); + IGN (1, s4100); + + IGN (1, "%s", "123"); + IGN (1, "%s", s4100); + + /* Output in excess of the maximum of 4095 bytes. */ + IGN (4097, s4100); + + IGN (4097, "%s", s4100); +} + + +void test_vsprintf (__builtin_va_list va) +{ +#undef IGN +#define IGN(fmt) __builtin_vsprintf (buffer, fmt, va); sink (buffer) + + /* All of the following calls to vsprintf must be emitted (and not + transformed into memcpy, strcpy, or similar). */ + + /* Output in excess of the maximum of 4095 bytes. */ + IGN (s4100); +} + + +void test_vsnprintf (__builtin_va_list va) +{ +#undef IGN +#define IGN(N, fmt) __builtin_vsnprintf (buffer, N, fmt, va); sink (buffer) + + /* All of the following calls to vsnprintf must be emitted (and not + transformed into memcpy, strcpy, or similar). */ + + /* Truncated output could be optimized into strncpy (not done yet). */ + IGN (1, "123"); + IGN (1, s4100); + + /* Output in excess of the maximum of 4095 bytes. */ + IGN (4097, s4100); +} + +/* { dg-final { scan-tree-dump-times "builtin_sprintf" 4 "optimized" } } + { dg-final { scan-tree-dump-times "builtin_snprintf" 6 "optimized" } } + { dg-final { scan-tree-dump-times "builtin_vsprintf" 1 "optimized" } } + { dg-final { scan-tree-dump-times "builtin_vsnprintf" 3 "optimized" } } */ + +#define S10 "0123456789" +#define S100 S10 S10 S10 S10 S10 S10 S10 S10 S10 S10 +#define S1000 S100 S100 S100 S100 S100 S100 S100 S100 S100 S100 + +const char s4100[4101] = S1000 S1000 S1000 S1000 S100; diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c index b4a9a6e..0242d94 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-1.c @@ -1068,7 +1068,7 @@ void test_sprintf_chk_e_const (void) void test_sprintf_chk_s_nonconst (int w, int p, const char *s) { T (-1, "%s", s); - T ( 0, "%s", s); /* { dg-warning "writing a terminating nul" } */ + T ( 0, "%-s", s); /* { dg-warning "writing a terminating nul" } */ T ( 1, "%s", s); T (-1, "%.0s", s); T ( 1, "%.0s", s); diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-2.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-2.c index f21dae4..60695b8 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-2.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-2.c @@ -94,8 +94,8 @@ struct Arrays { void test_s_nonconst (int w, int p, const char *s, const wchar_t *ws, struct Arrays *a) { - T (0, "%s", s); /* { dg-warning "into a region" "sprintf transformed into strcpy" { xfail *-*-* } } */ - T (1, "%s", s); /* { dg-warning "nul past the end" "sprintf transformed into strcpy" { xfail *-*-* } } */ + T (0, "%s", s); /* { dg-warning "into a region" } */ + T (1, "%s", s); /* { dg-warning "nul past the end" } */ T (1, "%1s", s); /* { dg-warning "writing a terminating nul" } */ T (1, "%.0s", s); T (1, "%.1s", s); /* { dg-warning "may write a terminating nul" } */ @@ -112,30 +112,27 @@ void test_s_nonconst (int w, int p, const char *s, const wchar_t *ws, T (1, "%.1ls", ws); /* { dg-warning "may write a terminating nul" } */ T (1, "%ls", ws); /* { dg-warning "may write a terminating nul" } */ - /* Verify that the size of the array is used in lieu of its length. - The minus sign disables GCC's sprintf to strcpy transformation. - In the case below, the length of s->a1 can be at most zero, so - the call should not be diagnosed. */ - T (1, "%-s", a->a1); + /* Verify that the size of the array is used in lieu of its length. */ + T (1, "%s", a->a1); /* In the following test, since the length of the strings isn't known, their type (the array) is used to bound the maximum length to 1, - which means the "%-s" directive would not overflow the buffer, + which means the "%s" directive would not overflow the buffer, but it would leave no room for the terminating nul. */ - T (1, "%-s", a->a2); /* { dg-warning "may write a terminating nul" } */ + T (1, "%s", a->a2); /* { dg-warning "may write a terminating nul" } */ /* Unlike in the test above, since the length of the string is bounded - by the array type to at most 2, the "^-s" directive is diagnosed firts, + by the array type to at most 2, the "%s" directive is diagnosed firts, preventing the diagnostic about the terminatinb nul. */ - T (1, "%-s", a->a3); /* { dg-warning "directive writing up to 2 bytes" } */ + T (1, "%s", a->a3); /* { dg-warning "directive writing up to 2 bytes" } */ /* The length of a zero length array and flexible array member is unknown and at leve 2 assumed to be at least 1. */ - T (1, "%-s", a->a0); /* { dg-warning "may write a terminating nul" } */ - T (1, "%-s", a->ax); /* { dg-warning "may write a terminating nul" } */ + T (1, "%s", a->a0); /* { dg-warning "may write a terminating nul" } */ + T (1, "%s", a->ax); /* { dg-warning "may write a terminating nul" } */ - T (2, "%-s", a->a0); - T (2, "%-s", a->ax); + T (2, "%s", a->a0); + T (2, "%s", a->ax); } /* Exercise buffer overflow detection with non-const integer arguments. */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c index d7d9317..fb1d2b7 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-warn-3.c @@ -56,10 +56,10 @@ void test_sprintf_chk_string (const char *s, const char *t) { #define x x () - T (1, "%s", x ? "" : "1"); /* { dg-warning "nul past the end" } */ - T (1, "%s", x ? "1" : ""); /* { dg-warning "nul past the end" } */ - T (1, "%s", x ? s : "1"); /* { dg-warning "nul past the end" } */ - T (1, "%s", x ? "1" : s); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "" : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : ""); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? s : "1"); /* { dg-warning "nul past the end" } */ + T (1, "%-s", x ? "1" : s); /* { dg-warning "nul past the end" } */ /* When neither string is known no warning should be issued at level 1 since their lenghts are assumed to be zero. */

handle sprintf(d, "%s", ...) in gimple-ssa-sprintf.c

Commit Message

Comments

Patch