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.
As we've discussed, the C/GIMPLE semantic issues mean we can not rely on
types. One could use that as an argument that the check is
fundamentally wrong, but in the case where things go screwy here we
always return NULL indicating we do not know the length. So it's the
safe thing to do.
On the other hand one could argue that the test is a canary that
something unexpected has happened, either in terms of the passed in
size, or the types of the argument. For the case above, we miss the
warning. Missing a warning is less worrisome than generating wrong code.
Another alternative (and I hate to suggest it) is to indicate via an
argument how the result of c_strlen is used. ie, is it used just for
warnings or is it used for opt/codegen. For the former we wouldn't use
the test. For the latter we would. That ultimately gets the warning
you want, but also keeps us safe for codegen/opt purposes.
Jeff
