<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Committed in r247029.<div class=""><br class=""></div><div class="">Thanks!</div><div class=""><br class=""></div><div class="">Steven</div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Sep 8, 2015, at 10:14 AM, Steven Wu via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><meta http-equiv="Content-Type" content="text/html charset=utf-8" class=""><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">I just measure all 4 method: memcpy, fortify source, no inline function and always inline function. None of them will have any measurable difference on the benchmark. I will go with disabling fortify source because it seems the most future proof. I will add a comment in the Makefile for the reason of turning off FORTIFY_SOURCE.<div class=""><br class=""></div><div class="">Steven</div><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Sep 7, 2015, at 7:29 PM, George Burgess IV <<a href="mailto:george.burgess.iv@gmail.com" class="">george.burgess.iv@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><span style="font-size:12.8000001907349px" class="">> That doesn’t sound very safe :).</span><div class=""><span style="font-size:12.8000001907349px" class=""><br class=""></span></div><div class=""><span style="font-size:12.8000001907349px" class="">That's why I opted for noinline first. :) </span></div><div class=""><span style="font-size:12.8000001907349px" class=""><br class=""></span></div><div class="">> <span style="font-size:12.8000001907349px" class="">Let me run some benchmarks </span><span class="" tabindex="0" style="font-size:12.8000001907349px"><span class="">tomorrow</span></span><span style="font-size:12.8000001907349px" class=""> and make sure there is no unexpected performance impact. I am leaning towards turn off FORTIFY or using memcpy</span></div><div class=""><span style="font-size:12.8000001907349px" class=""><br class=""></span></div><div class=""><span style="font-size:12.8000001907349px" class="">SGTM. </span></div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Mon, Sep 7, 2015 at 7:14 PM, Steven Wu <span dir="ltr" class=""><<a href="mailto:stevenwu@apple.com" target="_blank" class="">stevenwu@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Sep 6, 2015, at 11:54 PM, George Burgess IV <<a href="mailto:george.burgess.iv@gmail.com" target="_blank" class="">george.burgess.iv@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Hm. We can also make _strcpy_nochk always_inline. It doesn't *guarantee* a failure of __builtin_object_size, but it does require clang to lower to @llvm.objectsize, which is what was happening when nothing was failing. This way, we shouldn't end up dropping any perf on the floor, and we have a guaranteed solution for all platforms until someone makes @llvm.objectsize substantially smarter.</div></div></blockquote></span>That doesn’t sound very safe :). And you can play with the inline threshold in LLVM and break the benchmark.<span class=""><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">WRT Disabling FORTIFY: Ubuntu's default libc + Android's Bionic seem to both use -D_FORTIFY_SOURCE=0 to mean "turn off FORTIFY", so it's probably okay to assume that your proposed change will work well on most (if not all) platforms, as well.</div><div class=""><br class=""></div><div class="">I'm happy with either solution. Please note that FORTIFY may slow things down a bit, so if we track things over time, then we might see a bit of a perf increase from the latter change.</div></div></div></blockquote></span>Let me run some benchmarks tomorrow and make sure there is no unexpected performance impact. I am leaning towards turn off FORTIFY or using memcpy.<div class=""><div class="h5"><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Sun, Sep 6, 2015 at 11:18 PM, Steven Wu <span dir="ltr" class=""><<a href="mailto:stevenwu@apple.com" target="_blank" class="">stevenwu@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">Turning off inlining will hurt the performance for the cases when the compiler can optimize strcpy functions and we do track performance for test-suite.<div class="">On apple platform, the minimal change can just be turning off FORTIFY_SOURCE for this benchmark. Do you know how your change will affect other platforms?</div><div class=""><br class=""></div><div class="">--- a/MultiSource/Benchmarks/MiBench/consumer-typeset/Makefile<br class="">+++ b/MultiSource/Benchmarks/MiBench/consumer-typeset/Makefile<br class="">@@ -1,7 +1,7 @@<br class=""> LEVEL = ../../../..<br class=""> <br class=""> PROG = consumer-typeset<br class="">-CPPFLAGS = -DOS_UNIX=1 -DOS_DOS=0 -DOS_MAC=0 -DDB_FIX=0 -DUSE_STAT=1 -DSAFE_DFT=0 -DCOLLATE=1 -DLIB_DIR=\"lout.lib\" -DFONT_DIR=\"font\" -DMAPS_DIR=\"maps\" -DINCL_DIR=\"include\" -DDATA_DIR=\"data\" -DHYPH_DIR=\"hyph\" -DLOCALE_DIR=\"locale\" -DCHAR_IN=1 -DCHAR_OUT=0 -DLOCALE_ON=1 -DASSERT_ON=1 -DDEBUG_ON=0 -DPDF_COMPRESSION=0<br class="">+CPPFLAGS = -DOS_UNIX=1 -DOS_DOS=0 -DOS_MAC=0 -DDB_FIX=0 -DUSE_STAT=1 -DSAFE_DFT=0 -DCOLLATE=1 -DLIB_DIR=\"lout.lib\" -DFONT_DIR=\"font\" -DMAPS_DIR=\"maps\" -DINCL_DIR=\"include\" -DDATA_DIR=\"data\" -DHYPH_DIR=\"hyph\" -DLOCALE_DIR=\"locale\" -DCHAR_IN=1 -DCHAR_OUT=0 -DLOCALE_ON=1 -DASSERT_ON=1 -DDEBUG_ON=0 -DPDF_COMPRESSION=0 -D_FORTIFY_SOURCE=0<br class=""> LDFLAGS = -lm<br class=""> RUN_OPTIONS = -x -I $(PROJ_SRC_DIR)/data/include -D $(PROJ_SRC_DIR)/data/data -F $(PROJ_SRC_DIR)/data/font -C $(PROJ_SRC_DIR)/data/maps -H $(PROJ_OBJ_DIR)/data/hyph $(PROJ_SRC_DIR)/large.lout<br class=""><br class=""></div><div class=""><div class=""><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Sep 6, 2015, at 10:40 PM, George Burgess IV <<a href="mailto:george.burgess.iv@gmail.com" target="_blank" class="">george.burgess.iv@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Well, we can lie by obfuscating things a bit. :)<div class=""><br class=""></div><div class="">__builtin_object_size relies heavily on inlining to work well across-functions. So, the following should make it always fail:<div class=""><br class=""></div><div class="">static void __attribute__((noinline)) _strcpy_nochk(char *dst, char *src) { strcpy(dst, src); }</div><div class="">#define StringCopy(a, b) _strcpy_nochk((char *)(a), (char *)(b))</div><div class=""><br class=""></div></div></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Sun, Sep 6, 2015 at 9:25 PM, Steven Wu <span dir="ltr" class=""><<a href="mailto:stevenwu@apple.com" target="_blank" class="">stevenwu@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class=""><br class=""><div class=""><span class=""><blockquote type="cite" class=""><div class="">On Sep 6, 2015, at 6:38 PM, George Burgess IV <<a href="mailto:george.burgess.iv@gmail.com" target="_blank" class="">george.burgess.iv@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class="">Thanks for catching this!</div><div class=""><br class=""></div>I'm not familiar with at all with test-suite, so someone else may be better suited to review your patch. That being said:<div class=""><br class=""></div><div class="">id3tag.c -- LGTM</div><div class=""><br class=""></div><div class="">externs.c -- Did some investigating. This code is rather different than what I'm used to. :)</div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">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: <a href="http://opensource.apple.com/source/Libc/Libc-1044.10.1/include/secure/_string.h" target="_blank" class="">http://opensource.apple.com/source/Libc/Libc-1044.10.1/include/secure/_string.h</a> )</div><div class=""><br class=""></div><div class="">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.</div><div class=""><br class=""></div><div class="">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. :)</div></div></div></blockquote><div class=""><br class=""></div></span>I guess there is no good way to lie to the type system that you can legally copy the string into 4 char array?<br class=""><div class="">How about we just #define StringCopy to __builtin___strcpy_chk with __builtin_object_size(p, 0)? It might break -fno-builtin build though.</div><span class=""><font color="#888888" class=""><div class=""><br class=""></div><div class="">Steven</div></font></span><div class=""><div class=""><div class=""><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Sep 4, 2015 at 11:25 PM, Steven Wu <span dir="ltr" class=""><<a href="mailto:stevenwu@apple.com" target="_blank" class="">stevenwu@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br class="">
<br class="">
Thanks<br class="">
<span class=""><font color="#888888" class=""><br class="">
Steven<br class="">
<br class="">
</font></span><br class=""><br class="">
<br class="">
From 7151bf31954c9d0680e4bb592dbc130d0234650b Mon Sep 17 00:00:00 2001<br class="">
From: Steven Wu <<a href="mailto:stevenwu@apple.com" target="_blank" class="">stevenwu@apple.com</a>><br class="">
Date: Fri, 4 Sep 2015 23:15:21 -0700<br class="">
Subject: [PATCH] Fix the undefined behavior in MiBench<br class="">
<br class="">
The undefined behavior is causing runtime failure after r246877.<br class="">
Fix consumer-lane by replacing strcpy with direct assignment to avoid<br class="">
buffer overflow. consumer-typeset is intentionally overflowing buffer so<br class="">
use memcpy instead of strcpy.<br class="">
---<br class="">
MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c | 2 +-<br class="">
MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h | 3 ++-<br class="">
2 files changed, 3 insertions(+), 2 deletions(-)<br class="">
<br class="">
diff --git a/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c b/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c<br class="">
index e24a966..23f2b86 100644<br class="">
--- a/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c<br class="">
+++ b/MultiSource/Benchmarks/MiBench/consumer-lame/id3tag.c<br class="">
@@ -34,7 +34,7 @@ void id3_inittag(ID3TAGDATA *tag) {<br class="">
strcpy( tag->album, "");<br class="">
strcpy( tag->year, "");<br class="">
strcpy( tag->comment, "");<br class="">
- strcpy( tag->genre, "ˇ"); /* unset genre */<br class="">
+ tag->genre[0] = 'ˇ'; /* unset genre */<br class="">
tag->track = 0;<br class="">
<br class="">
tag->valid = 0; /* not ready for writing*/<br class="">
diff --git a/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h b/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h<br class="">
index 7e050bd..a14e8ee 100644<br class="">
--- a/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h<br class="">
+++ b/MultiSource/Benchmarks/MiBench/consumer-typeset/externs.h<br class="">
@@ -3266,7 +3266,8 @@ extern int strcollcmp(char *a, char *b);<br class="">
( UseCollate ? strcollcmp((char *)(a),(char *)(b)) <= 0 \<br class="">
: strcmp((char *)(a),(char *)(b)) <= 0 )<br class="">
#define StringCat(a, b) strcat((char *)(a),(char *)(b))<br class="">
-#define StringCopy(a, b) strcpy((char *)(a),(char *)(b))<br class="">
+#define StringCopy(a, b) (char*)memcpy((void *)(a),(void *)(b), \<br class="">
+ strlen((char*)(b)) + 1)<br class="">
#define StringLength(a) strlen((char *)(a))<br class="">
#define StringFOpen(a, b) fopen( (char *) (a), (b) )<br class="">
#define StringFPuts(a, b) fputs( (char *) (a), (b) )<br class="">
--<br class="">
2.3.8 (Apple Git-58)<br class="">
<br class="">
<br class="">
<br class="">
<br class=""></blockquote></div><br class=""></div>
</div></blockquote></div></div></div><br class=""></div></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div></div></div></div></blockquote></div><br class=""></div>
</div></blockquote></div></div></div><br class=""></div></blockquote></div><br class=""></div>
</div></blockquote></div><br class=""></div></div>_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" class="">llvm-commits@lists.llvm.org</a><br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></div></body></html>