From: Martin Sebor <msebor@gmail.com>
To: Jeff Law <law@redhat.com>, gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] extend -Wstringop-overflow to allocated objects (PR 91582)
Date: Thu, 05 Dec 2019 01:37:00 -0000 [thread overview]
Message-ID: <dfbcf998-5502-d1b0-6e49-097d2bfe233a@gmail.com> (raw)
In-Reply-To: <432df580-5630-a6ff-581d-731222a34669@redhat.com>
On 12/2/19 10:06 AM, Jeff Law wrote:
> On 11/8/19 3:11 PM, Martin Sebor wrote:
>> Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow
>> doesn't consider out-of-bounds accesses to objects allocated
>> by alloca, malloc, other functions declared with attribute
>> alloc_size, or even VLAs with variable bounds. This was
>> a known limitation of the checks (done just before expansion)
>> relying on the the object size pass when they were introduced
>> in GCC 7.
>>
>> But since its introduction in GCC 7, the warning has evolved
>> beyond some of the limitations of the object size pass. Unlike
>> it, the warning considers non-constant offsets and stores with
>> non-constant sizes. Attached is a simple enhancement that
>> (finally) adds the ability to also detect overflow in allocated
>> objects to the warning.
>>
>> With the patch GCC detects the overflow in code like this:
>>
>> Â char* f (void)
>> Â {
>> Â Â Â char s[] = "12345";
>> Â Â Â char *p = malloc (strlen (s));
>> Â Â Â strcpy (p, s);Â Â // warning here
>> Â Â Â return p;
>> Â }
>>
>> but not (yet) in something like this:
>>
>> Â char* g (const char *s)
>> Â {
>> Â Â Â char *p = malloc (strlen (s));
>> Â Â Â strcpy (p, s);Â Â // no warning (yet)
>> Â Â Â return p;
>> Â }
>>
>> and quite a few other examples. Doing better requires extending
>> the strlen pass. I'm working on this extension and expect to
>> submit a patch before stage 1 ends.
>>
>> Martin
>>
>> PS I was originally planning to do all the allocation checking
>> in the strlen pass but it occurred to me that by also enhancing
>> the compute_objsize function, all warnings that use it will
>> benefit. Besides -Wstringop-overflow this includes a subset
>> of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's
>> nice when a small enhancement has such a broad positive effect.
>
>> PR middle-end/91582 - missing heap overflow detection for strcpy
>>
>> gcc/ChangeLog:
>>
>> * builtins.c (gimple_call_alloc_size): New function.
>> (compute_objsize): Add argument. Call gimple_call_alloc_size.
>> Handle variable offsets and indices.
>> * builtins.h (gimple_call_alloc_size): Declare.
>> (compute_objsize): Add argument.
>> * tree-ssa-strlen.c (handle_store): Handle calls to allocated objects.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * c-c++-common/Wstringop-truncation.c: Remove xfails.
>> * gcc/testsuite/g++.dg/ext/attr-alloc_size.C: Suppress -Warray-bounds.
>> * gcc.dg/Wstringop-overflow-22.c: New test.
>> * gcc/testsuite/gcc.dg/attr-alloc_size.c: Suppress -Warray-bounds.
>> * gcc/testsuite/gcc.dg/attr-copy-2.c: Same.
>> * gcc.dg/builtin-stringop-chk-5.c: Remove xfails.
>> * gcc.dg/builtin-stringop-chk-8.c: Same. Correct the text of expected
>> warnings.
>> * gcc.target/i386/pr82002-2a.c: Prune expected warning.
>> * gcc.target/i386/pr82002-2b.c: Same.
> [ ... ]
>
>
>> Index: gcc/builtins.c
>> ===================================================================
>> --- gcc/builtins.c (revision 277978)
>> +++ gcc/builtins.c (working copy)
>> @@ -3563,6 +3563,80 @@ check_access (tree exp, tree, tree, tree dstwrite,
>> return true;
>> }
>>
>> +/* If STMT is a call to an allocation function, returns the size
>> + of the object allocated by the call. */
>> +
>> +tree
>> +gimple_call_alloc_size (gimple *stmt)
>> +{
>> + tree size = gimple_call_arg (stmt, argidx1);
>> + tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : integer_one_node;
>> +
>> + /* To handle ranges do the math in wide_int and return the product
>> + of the upper bounds as a constant. Ignore anti-ranges. */
>> + wide_int rng1[2];
>> + if (TREE_CODE (size) == INTEGER_CST)
>> + rng1[0] = rng1[1] = wi::to_wide (size);
>> + else if (TREE_CODE (size) != SSA_NAME
>> + || get_range_info (size, rng1, rng1 + 1) != VR_RANGE)
>> + return NULL_TREE;
>> +
>> + wide_int rng2[2];
>> + if (TREE_CODE (n) == INTEGER_CST)
>> + rng2[0] = rng2[1] = wi::to_wide (n);
>> + else if (TREE_CODE (n) != SSA_NAME
>> + || get_range_info (n, rng2 + 1, rng2 + 1) != VR_RANGE)
>> + return NULL_TREE;
> Should that 2nd call to get_range_info be "get_range_info (n, rng2, rng2
> + 1)? I don't think it makes any difference in practice due to the
> implementation of get_range_info, but if it wasn't intentional let's get
> it fixed.
Yes, it should be. It's correct in my tree but didn't post
the updated revision.
>
>
>> Index: gcc/builtins.h
>> ===================================================================
>> --- gcc/builtins.h (revision 277978)
>> +++ gcc/builtins.h (working copy)
>> @@ -134,7 +134,8 @@ extern tree fold_call_stmt (gcall *, bool);
>> extern void set_builtin_user_assembler_name (tree decl, const char *asmspec);
>> extern bool is_simple_builtin (tree);
>> extern bool is_inexpensive_builtin (tree);
>> -extern tree compute_objsize (tree, int, tree * = NULL);
>> +tree gimple_call_alloc_size (gimple *);
>> +extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL);
>>
>> extern bool readonly_data_expr (tree exp);
>> extern bool init_target_chars (void);
> Is there a reason there's no "extern" on the gimple_call_alloc_size
> prototype?
I'm sure it was copied and pasted from the definition. It makes
no difference either way so it didn't get caught by anything.
>
> I think this is fine with those nits fixed. You'll have a minor merge
> conflict with the compute_objsize changes due to recent fixes in the
> same hunk of code, but I don't think it warrants reposting/resubmission.
>
I've fixed the nits above and committed r278983 after retesting.
This is an improvement in the buffer overflow detection but there
is still the (arguably more important) second half of it:
extending the strlen pass to detect the overflow that cannot be
caught later (e.g., all stores by MEM_REFs are only handled in
strlen): https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02340.html
Martin
next prev parent reply other threads:[~2019-12-05 1:37 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-08 22:11 Martin Sebor
2019-11-18 18:23 ` [PING][PATCH] " Martin Sebor
2019-11-25 17:54 ` [PING 2][PATCH] " Martin Sebor
2019-12-02 17:06 ` [PATCH] " Jeff Law
2019-12-05 1:37 ` Martin Sebor [this message]
2019-12-06 15:44 ` Christophe Lyon
2019-12-06 17:03 ` Martin Sebor
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dfbcf998-5502-d1b0-6e49-097d2bfe233a@gmail.com \
--to=msebor@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).