<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2015-05-20 11:36 GMT-07:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5"><br>
> On 2015 May 20, at 11:24, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
><br>
>> On Wed, May 20, 2015 at 11:13 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>> > On 2015 May 20, at 09:59, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
>> ><br>
>> > Hi dexonsmith, rafael,<br>
>> ><br>
>> > This patch modifies the memory buffer creation in the AsmParser library so that is requires a terminating null character.<br>
>> > This change is needed because LLLexer in AsmParser checks for EOF when it sees the null character,<br>
>><br>
>> Is this a bug? Should LLexer be fixed instead?<br>
><br>
> Clang requires a null terminator. It lets you skip an awful lot of range checks, so enforcing null termination might be the better change. Typically all you have to do is mmap filesize + 1.<br>
<br>
</div></div>Well, mine was just a gut reaction. If Matthias is okay with it,<br>
I am too.<br>
<br>
@Alex, have a look -- if you think it'll add "a lot" of range checks<br>
to LLexer, then I'm fine with the patch mostly as is. But if it's<br>
pretty simple then we might as well support `StringRef` fully.<br></blockquote><div><br></div><div>It only adds one range check in the '<span style="color:rgb(0,0,0)">getNextChar' method. It's also</span></div><div><span style="color:rgb(0,0,0)">the only method that has to be modified. But I'm </span><span style="color:rgb(0,0,0)">not sure what </span></div><div><span style="color:rgb(0,0,0)">performance impact it would have, although there's </span><span style="color:rgb(0,0,0)">already a check for </span></div><div><span style="color:rgb(0,0,0)">a null character in that method.</span></div><div><span style="color:rgb(0,0,0)">I think I'll go ahead and submit a range check patch for now.</span></div><div><span style="color:rgb(0,0,0)"><br></span></div><div><span style="color:rgb(0,0,0)">Alex.</span></div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
If we go with your original patch, I have a comment on the test.<br>
<br>
> Index: unittests/AsmParser/AsmParserTest.cpp<br>
> ===================================================================<br>
> --- /dev/null<br>
> +++ unittests/AsmParser/AsmParserTest.cpp<br>
> @@ -0,0 +1,37 @@<br>
> +//===- llvm/unittest/AsmParser/AsmParserTest.cpp - asm parser unittests ---===//<br>
> +//<br>
> +// The LLVM Compiler Infrastructure<br>
> +//<br>
> +// This file is distributed under the University of Illinois Open Source<br>
> +// License. See LICENSE.TXT for details.<br>
> +//<br>
> +//===----------------------------------------------------------------------===//<br>
> +<br>
> +#include "llvm/ADT/StringRef.h"<br>
> +#include "llvm/AsmParser/Parser.h"<br>
> +#include "llvm/IR/LLVMContext.h"<br>
> +#include "llvm/IR/Module.h"<br>
> +#include "llvm/Support/SourceMgr.h"<br>
> +#include "gtest/gtest.h"<br>
> +<br>
> +using namespace llvm;<br>
> +<br>
> +namespace {<br>
> +<br>
> +#ifdef GTEST_HAS_DEATH_TEST<br>
> +#ifndef NDEBUG<br>
> +<br>
> +TEST(AsmParserTest, NonNullTerminatedInput) {<br>
> + LLVMContext &Ctx = getGlobalContext();<br>
> + StringRef Source = "; Empty module \n\1\2";<br>
> + SMDiagnostic Error;<br>
> + std::unique_ptr<Module> Mod;<br>
> + EXPECT_DEATH(Mod = parseAssemblyString(Source.substr(0, Source.size() - 2),<br>
> + Error, Ctx),<br>
> + "Buffer is not null terminated!");<br>
> +}<br>
> +<br>
> +#endif<br>
> +#endif<br>
<br>
I think you should add another test that always runs, which has a<br>
properly null-terminated comment and checks for sane results.<br>
<br>
> +<br>
> +} // end anonymous namespace<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div>