[PATCH] Add a character size parameter to c_strlen/get_range_strlen

[Martin Sebor]

On 08/20/2018 04:17 PM, Jeff Law wrote:
> On 08/18/2018 11:38 AM, Martin Sebor wrote:
>> On 08/17/2018 09:32 PM, Jeff Law wrote:
>>> On 08/17/2018 02:17 PM, Martin Sebor wrote:
>>>> On 08/17/2018 12:44 PM, Bernd Edlinger wrote:
>>>>> On 08/17/18 20:23, Martin Sebor wrote:
>>>>>> On 08/17/2018 06:14 AM, Joseph Myers wrote:
>>>>>>> On Fri, 17 Aug 2018, Jeff Law wrote:
>>>>>>>
>>>>>>>> On 08/16/2018 05:01 PM, Joseph Myers wrote:
>>>>>>>>> On Thu, 16 Aug 2018, Jeff Law wrote:
>>>>>>>>>
>>>>>>>>>> restores previous behavior.  The sprintf bits want to count
>>>>>>>>>> element
>>>>>>>>>> sized chunks, which for wchars is 4 bytes (that count will then be
>>>>>>>>>
>>>>>>>>>>    /* Compute the range the argument's length can be in.  */
>>>>>>>>>> -  fmtresult slen = get_string_length (arg);
>>>>>>>>>> +  int count_by = dir.specifier == 'S' || dir.modifier ==
>>>>>>>>>> FMT_LEN_l ? 4 : 1;
>>>>>>>>>
>>>>>>>>> I don't see how a hardcoded 4 is correct here.  Surely you need to
>>>>>>>>> example
>>>>>>>>> wchar_type_node to determine its actual size for this target.
>>>>>>>> We did kick this around a little.  IIRC Martin didn't think that it
>>>>>>>> was
>>>>>>>> worth handling the 2 byte wchar case.
>>>>>>
>>>>>> Sorry, I think we may have miscommunicated -- I didn't think it
>>>>>> was useful to pass a size of the character type to the function.
>>>>>> I agree that passing in a hardcoded constant doesn't seem right
>>>>>> (even if GCC's wchar_t were always 4 bytes wide).
>>>>>>
>>>>>> I'm still not sure I see the benefit of passing in the expected
>>>>>> element size given that the patch causes c_strlen() fail when
>>>>>> the actual element size doesn't match ELTSIZE.  If the caller
>>>>>> asks for the number of bytes in a const wchar_t array it should
>>>>>> get back the number bytes.  (I could see it fail if the caller
>>>>>> asked for the number of words in a char array whose size was
>>>>>> not evenly divisible by wordsize.)
>>>>>>
>>>>>
>>>>> I think in this case c_strlen should use the type which the %S format
>>>>> uses at runtime, otherwise it will not have anything to do with
>>>>> the reality.
>>>>
>>>> %S is not what I'm talking about.
>>>>
>>>> Failing in the case I described (a caller asking for the size
>>>> in bytes of a constant object whose type is larger) prevents
>>>> callers that don't care about the type from detecting the actual
>>>> size of a constant.
>>>>
>>>> Specifically for sprintf, it means that the buffer overflow
>>>> below is no longer diagnosed:
>>>>
>>>>   struct S { char a[2]; void (*pf)(void); };
>>>>
>>>>   void f (struct S *p)
>>>>   {
>>>>     const char *q = (char*)L"\x41424344\x45464748";
>>>>
>>>>     sprintf (p->a, "%s", q);
>>>>   }
>>> I don't think this is in the testsuite, is it?  I verified that there
>>> was no regressions when I installed Bernd's patch and when I installed
>>> yours.
>>
>> No, there are very few tests that exercise these kinds of mixed
>> argument types.  Code like that is most likely the result of
>> a mistake, but it's not the kind of a bug I had even thought
>> about until some of the codegen issues with mixed argument types
>> were brought up (PR 86711/86714).
> Phew.  I was worried I'd somehow missed the failure or tested the wrong
> patch or who knows what.
>
> Can you add that test, xfailed for now to the testsuite?

Done in r263676.

>> I would just like the ability to get the length/size somehow
>> so that the questionable code that started us down this path
>> can be detected.  This is not just the sprintf(d, "%s", L"1")
>> kind of a mismatch but also the missing nul detection in cases
>> like:
> [ ... ]
> I know.  And ideally we'll be able to handle everything you want to.
> But we also have to realize that sometimes we may have to punt.
>
> Also remember that incremental progress is (usually) good.
>
>
>>
>>   const wchar_t a[1] = L"\xffffffff";
>>   strcpy(d, (char*)a);
> This touches on both the representational issue (excess elements in the
> initializer) and how to handle a non-terminated string.  Both are issues
> we're debating right now.
>
>>
>> I don't think this is necessarily an important use case to
>> optimize for but it is one that GCC optimizes already nonetheless,
>> and always has.  For example, this has always been folded into 4
>> by GCC and still is even with the patch:
>>
>>   const __WCHAR_TYPE__ wc[] = L"\x12345678";
>>
>>   int f (void)
>>   {
>>     const char *p = (char*)wc;
>>     return strlen (p);            // folded to 4
>>   }
>>
>> It's optimized because fold_const_call() relies on c_getstr()
>> which returns the underlying byte representation of the wide
>> string, and despite c_strlen() now trying to prevent it.
> And I think you're hitting on some of issues already raised in the thread.
>
> In this specific case though ISTM 4 is the right answer.  We casted to a
> char *, so that's what we should be counting.  Or am I missing
> something?  Also note that's the value we'd get from the strlen C
> library call IIUC.

It is the right answer.  My point is that this is optimized
but the change to c_strlen() prevents the same optimization
in other similar cases.  For example, GCC 6 folds this into
memcpy:

   __builtin_strcpy (d, (char*)L"\x12345678");

GCC 7 and 8 do too but get the byte count wrong (my bad).

Current trunk doesn't optimize it.  If restoring the original
behavior is the intent (and not just fixing the counting bug)
then c_strlen() should be fixed to fold this again.

(I'm not a fan of the strcpy to memcpy transformation because
it makes other things difficult but that's beside the point.)

>> The missing nul variant of the same test case isn't folded (it
>> ends up calling the library strlen) but the bug cannot be
>> detected using the modified c_strlen():
>>
>>   const __WCHAR_TYPE__ wc[1] = L"\x12345678";   // no nul
>>
>>   int f (void)
>>   {
>>     const char *p = (char*)wc;
>>     return strlen (p);            // not folded
>>   }
>>
>> To detect it, I'd have to use some other function, like
>> c_getstr().  I could certainly do that but I don't think
>> I should need to.
> And that's on the list of things we're going to try and address next.  I
> don't think it needs to be conflated with change which indicated to
> c_strlen how to count.

The two are one and the same: if c_strlen() doesn't count bytes
in arrays of wide characters (wchar_t, char16_t, or char32_t)
then the original GCC behavior is not restored and the missing
nul detection won't work for these "mixed type" cases.

> I do hope you're getting all these testcases recorded.  I'd even support
> installing them into the suite now, properly xfailed.  It's easier to
> see how any patch under consideration affects the wider set of tests.

I haven't been recording any of them --  I have no idea where
this is going.  As I've said, these patches prevent some of
the work I have already submitted.  That's why I have been
objecting to them, including adding the ELTSIZE argument to
c_strlen().

Martin