On Thu, Feb 8, 2018 at 7:12 PM, Martin Sebor <mse...@gmail.com> wrote:
> On 02/08/2018 07:39 AM, Richard Biener wrote:
>>
>> On Thu, Feb 8, 2018 at 6:35 AM, Jeff Law <l...@redhat.com> wrote:
>>>
>>> On 02/06/2018 05:57 AM, Jakub Jelinek wrote:
>>>>
>>>> On Tue, Feb 06, 2018 at 01:46:21PM +0100, Marek Polacek wrote:
>>>>>
>>>>> --- gcc/testsuite/c-c++-common/Wstringop-truncation-3.c
>>>>> +++ gcc/testsuite/c-c++-common/Wstringop-truncation-3.c
>>>>> @@ -0,0 +1,20 @@
>>>>> +/* PR tree-optimization/84228 */
>>>>> +/* { dg-do compile } */
>>>>> +/* { dg-options "-Wstringop-truncation -O2 -g" } */
>>>>> +
>>>>> +char *strncpy (char *, const char *, __SIZE_TYPE__);
>>>>> +struct S
>>>>> +{
>>>>> + char arr[64];
>>>>> +};
>>>>> +
>>>>> +int
>>>>> +foo (struct S *p1, const char *a)
>>>>> +{
>>>>> + if (a)
>>>>> + goto err;
>>>>> + strncpy (p1->arr, a, sizeof p1->arr); /* { dg-bogus "specified
>>>>> bound" } */
>>>>
>>>>
>>>> Just curious, what debug stmt is in between those?
>>>> Wouldn't it be better to force them a couple of debug stmts?
>>>> Say
>>>> int b = 5, c = 6, d = 7;
>>>> at the start of the function and
>>>> b = 8; c = 9; d = 10;
>>>> in between strncpy and the '\0' store?
>>>>
>>>>> + p1->arr[3] = '\0';
>>>>> +err:
>>>>> + return 0;
>>>>> +}
>>>>> diff --git gcc/tree-ssa-strlen.c gcc/tree-ssa-strlen.c
>>>>> index c3cf432a921..f0f6535017b 100644
>>>>> --- gcc/tree-ssa-strlen.c
>>>>> +++ gcc/tree-ssa-strlen.c
>>>>> @@ -1849,7 +1849,7 @@ maybe_diag_stxncpy_trunc (gimple_stmt_iterator
>>>>> gsi, tree src, tree cnt)
>>>>>
>>>>> /* Look for dst[i] = '\0'; after the stxncpy() call and if found
>>>>> avoid the truncation warning. */
>>>>> - gsi_next (&gsi);
>>>>> + gsi_next_nondebug (&gsi);
>>>>> gimple *next_stmt = gsi_stmt (gsi);
>>>>>
>>>>> if (!gsi_end_p (gsi) && is_gimple_assign (next_stmt))
>>>>
>>>>
>>>> Ok for trunk, though generally looking at just next stmt is very
>>>> fragile, might be
>>>> better to look at the strncpy's vuse immediate uses if they are within
>>>> the
>>>> same basic block and either don't alias with it, or are the store it is
>>>> looking for, or something similar.
>>>
>>> Martin and I wandered down this approach a bit and ultimately decided
>>> against it. While yes it could avoid a false positive by looking at the
>>> immediate uses, but I'm not sure avoiding the false positive in those
>>> cases is actually good!
>>>
>>> THe larger the separation between the strcpy and the truncation the more
>>> likely it is that the code is wrong or at least poorly written and
>>> deserves a developer looksie.
>>
>>
>> But it's the next _GIMPLE_ stmt it looks at. Make it
>>
>> char d[3];
>>
>> void f (const char *s, int x)
>> {
>> char d[x];
>> __builtin_strncpy (d, s, sizeof d);
>> d[x-1] = 0;
>> }
>>
>> and I bet it will again warn since x-1 is a separate GIMPLE stmt.
>
>
> It doesn't but only because it doesn't know how to handle VLAs.
>
>> The patch is of course ok but still... simply looking at all
>> immediate uses of the VDEF of the strncpy call, stopping
>> at the single stmt with the "next" VDEF should be better
>> (we don't have a next_vdef () helper).
>
>
> IIRC, one of the problems I ran into was how to handle code
> like this:
>
> void f (const char *s)
> {
> char d[8];
> char *p = strncpy (d, s, sizeof d);
>
> foo (p);
>
> d[7] = 0;
> }
>
> I.e., having to track all pointers to d between the call to
> strncpy and the assignment of the nul and make sure none of
> them ends up used in a string function. It didn't seem
> the additional complexity would have been worth the effort
> (or the likely false negatives).
Well, I'd just walk immediate uses of the VDEF of the
strncpy call, not of the pointer argument. There will be exactly _one_ possible
store (gimple_vdef () is non-NULL) that you need to verify (with using
the current matching
logic). But it'll skip non-store statements for you.
Richard.
> Martin
>
