Fixing the test-suite broken after r246877

Steven Wu via llvm-commits llvm-commits at lists.llvm.org
Sun Sep 6 21:25:57 PDT 2015


> 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 <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 <mailto: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 <mailto: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/3f401a51/attachment.html>


More information about the llvm-commits mailing list