Bug 104119 - [12 Regression] unexpected -Wformat-overflow after strlen in ILP32 since Ranger integration
Summary: [12 Regression] unexpected -Wformat-overflow after strlen in ILP32 since Rang...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 12.0
: P1 normal
Target Milestone: 12.0
Assignee: Martin Sebor
URL:
Keywords: diagnostic, patch
Depends on:
Blocks: Wformat-overflow
  Show dependency treegraph
 
Reported: 2022-01-19 14:54 UTC by Martin Liška
Modified: 2022-02-03 20:37 UTC (History)
4 users (show)

See Also:
Host:
Target: ILP32
Build:
Known to work:
Known to fail:
Last reconfirmed: 2022-01-19 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Liška 2022-01-19 14:54:58 UTC
Since the revision I see (reduced from multipath-tools package)

$ cat devmapper.i
struct {
  char id[8];
  int needs_paths_uevent;
} *p;

void dm_addmap_create()
{
  char *str = __builtin_malloc((sizeof("mpath-") - 1) + __builtin_strlen(p->id) + 10);
  __builtin_sprintf(str, "mpath-%s", p->id);
}

$ gcc devmapper.i -c -m32 -O2 -Werror=format-overflow
devmapper.i: In function ‘dm_addmap_create’:
devmapper.i:9:33: error: ‘%s’ directive writing up to 2147483644 bytes into a region of size 2147483641 [-Werror=format-overflow=]
    9 |   __builtin_sprintf(str, "mpath-%s", p->id);
      |                                 ^~
devmapper.i:9:3: note: ‘__builtin_sprintf’ output between 7 and 2147483651 bytes into a destination of size 2147483647
    9 |   __builtin_sprintf(str, "mpath-%s", p->id);
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors

Note one needs -m32!
Comment 1 Martin Sebor 2022-01-19 17:44:22 UTC
There is a subtle difference between GCC 11 and GCC 12 in the range determined for the length of the string argument to the %s directive that causes the warning to trigger.  In GCC 11, EVRP reports it's [0, 2147483645] while in GCC 12 Ranger reports it's [0, 2147483644].  The difference of 1 between the upper bounds is due to the improved range info (in both GCC 11 and 12 the strlen pass sets the upper bound to 2147483644 but only in GCC 12 does Ranger report the strlen result).

That being said, for the purposes of warning (but not optimization), when the string length cannot be accurately determined, I think the sprintf pass should use the size of the array the string is stored in.  So the warning in this case should assume the result of strlen(p->id) is in [0, 7].  That would avoid it in this instance and probably in quite a few others.
Comment 2 Andrew Pinski 2022-01-19 17:59:47 UTC
Hmm, very similar to PR 96367.
Comment 3 Martin Sebor 2022-01-19 18:07:25 UTC
Let me try to handle this for GCC 12.
Comment 5 Richard Biener 2022-01-20 07:43:29 UTC
I think the issue is that we saturate the buffer length but then subtract "mpath-" from that.
Comment 6 CVS Commits 2022-02-03 20:31:18 UTC
The master branch has been updated by Martin Sebor <msebor@gcc.gnu.org>:

https://gcc.gnu.org/g:3c9f762ad02f398c27275688c3494332f69237f5

commit r12-7033-g3c9f762ad02f398c27275688c3494332f69237f5
Author: Martin Sebor <msebor@redhat.com>
Date:   Thu Feb 3 13:27:16 2022 -0700

    Constrain conservative string lengths to array sizes [PR104119].
    
    Resolves:
    PR tree-optimization/104119 - unexpected -Wformat-overflow after strlen in ILP32 since Ranger integration
    
    gcc/ChangeLog:
    
            PR tree-optimization/104119
            * gimple-ssa-sprintf.cc (struct directive): Change argument type.
            (format_none): Same.
            (format_percent): Same.
            (format_integer): Same.
            (format_floating): Same.
            (get_string_length): Same.
            (format_character): Same.
            (format_string): Same.
            (format_plain): Same.
            (format_directive): Same.
            (compute_format_length): Same.
            (handle_printf_call): Same.
            * tree-ssa-strlen.cc (get_range_strlen_dynamic): Same.   Call
            get_maxbound.
            (get_range_strlen_phi): Same.
            (get_maxbound): New function.
            (strlen_pass::get_len_or_size): Adjust to parameter change.
            * tree-ssa-strlen.h (get_range_strlen_dynamic): Change argument type.
    
    gcc/testsuite/ChangeLog:
    
            PR tree-optimization/104119
            * gcc.dg/tree-ssa/builtin-snprintf-13.c: New test.
            * gcc.dg/tree-ssa/builtin-sprintf-warn-29.c: New test.
Comment 7 Martin Sebor 2022-02-03 20:37:11 UTC
The warning has been avoided in this case by using the size of the source array as the upper bound.  The heuristic the warning uses is still in place so when the size of the source array isn't known (e.g., when it's a flexible array member) it will still trigger.