Fixing the test-suite broken after r246877

[George Burgess IV via llvm-commits]

Well, we can lie by obfuscating things a bit. :)

__builtin_object_size relies heavily on inlining to work well
across-functions. So, the following should make it always fail:

static void __attribute__((noinline)) _strcpy_nochk(char *dst, char *src) {
strcpy(dst, src); }
#define StringCopy(a, b) _strcpy_nochk((char *)(a), (char *)(b))


On Sun, Sep 6, 2015 at 9:25 PM, Steven Wu <stevenwu at apple.com> wrote:

>
> On Sep 6, 2015, at 6:38 PM, George Burgess IV <george.burgess.iv at gmail.com>
> wrote:
>
> Thanks for catching this!
>
> I'm not familiar with at all with test-suite, so someone else may be
> better suited to review your patch. That being said:
>
> id3tag.c -- LGTM
>
> externs.c -- Did some investigating. This code is rather different than
> what I'm used to. :)
>
>
> From the compiler's perspective: string(OBJECT) returns a pointer to a
> 4-char array inside of OBJECT. We try to copy a 119-byte string (file path)
> into this. memcpy() works here because Apple uses __builtin_object_size(p,
> 0) for memcpy, and __builtin_object_size(p, 1) (sometimes 0) for strcpy;
> the former has no clue what size OBJECT is, so it fails, while the latter
> says "hey, this field is clearly 4 bytes," causing FORTIFY to kill the
> program when we try to copy more than that. (relevant header:
> http://opensource.apple.com/source/Libc/Libc-1044.10.1/include/secure/_string.h
> )
>
> From the program's perspective at runtime, OBJECT points to a non-constant
> sized malloc. NewWord guarantees that *OBJECT has sufficient space to store
> the entire string. The types lie.
>
> In light of this, if we could add a comment to the new StringCopy
> definition that reads something like: "Types lie at some points in this
> program, and FORTIFY implementations rely on types. Using memcpy instead of
> strcpy here makes some FORTIFY impls permissive enough that they won't
> crash us when we're doing tricky (but still correct) things," that may be
> best. :)
>
>
> I guess there is no good way to lie to the type system that you can
> legally copy the string into 4 char array?
> How about we just #define StringCopy to __builtin___strcpy_chk with
>  __builtin_object_size(p, 0)? It might break -fno-builtin build though.
>
> Steven
>
>
>
> On Fri, Sep 4, 2015 at 11:25 PM, Steven Wu <stevenwu at apple.com> wrote:
>
>> r246877 exposes some undefined behavior in LNT that cause the test to
>> crash. Both consumer-lame and consumer-typeset has strcpy that overflow the
>> buffer, one intentionally one maybe by accident. Here is my proposed way to
>> fix the tests.
>>
>> Thanks
>>
>> Steven
>>
>>
>>
>>
>> From 7151bf31954c9d0680e4bb592dbc130d0234650b Mon Sep 17 00:00:00 2001
>> From: Steven Wu <stevenwu at apple.com>
>> Date: Fri, 4 Sep 2015 23:15:21 -0700
>> Subject: [PATCH] Fix the undefined behavior in MiBench
>>
>> The undefined behavior is causing runtime failure after r246877.
>> Fix consumer-lane by replacing strcpy with direct assignment to avoid
>> buffer overflow. consumer-typeset is intentionally overflowing buffer so
>> use memcpy instead of strcpy.
>> ---
>>  MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c     | 2 +-
>>  MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h | 3 ++-
>>  2 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
>> b/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
>> index e24a966..23f2b86 100644
>> --- a/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
>> +++ b/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c
>> @@ -34,7 +34,7 @@ void id3_inittag(ID3TAGDATA *tag) {
>>         strcpy( tag->album, "");
>>         strcpy( tag->year, "");
>>         strcpy( tag->comment, "");
>> -       strcpy( tag->genre, "ˇ");       /* unset genre */
>> +       tag->genre[0] = 'ˇ';    /* unset genre */
>>         tag->track = 0;
>>
>>         tag->valid = 0;         /* not ready for writing*/
>> diff --git a/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h
>> b/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h
>> index 7e050bd..a14e8ee 100644
>> --- a/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h
>> +++ b/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h
>> @@ -3266,7 +3266,8 @@ extern int          strcollcmp(char *a, char *b);
>>                     ( UseCollate ? strcollcmp((char *)(a),(char *)(b)) <=
>> 0 \
>>                                  : strcmp((char *)(a),(char *)(b)) <= 0 )
>>  #define                  StringCat(a, b)       strcat((char *)(a),(char
>> *)(b))
>> -#define                  StringCopy(a, b)      strcpy((char *)(a),(char
>> *)(b))
>> +#define                  StringCopy(a, b)      (char*)memcpy((void
>> *)(a),(void *)(b), \
>> +                                             strlen((char*)(b)) + 1)
>>  #define                  StringLength(a)       strlen((char *)(a))
>>  #define                  StringFOpen(a, b)     fopen( (char *) (a), (b) )
>>  #define                  StringFPuts(a, b)     fputs( (char *) (a), (b) )
>> --
>> 2.3.8 (Apple Git-58)
>>
>>
>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150906/99847a4f/attachment.html>