[PATCH] AsmParser: Require null terminator in the created memory buffer.

[Duncan P. N. Exon Smith]

> On 2015 May 20, at 11:24, Reid Kleckner <rnk at google.com> wrote:
> 
>> On Wed, May 20, 2015 at 11:13 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> > On 2015 May 20, at 09:59, Alex Lorenz <arphaman at gmail.com> wrote:
>> >
>> > Hi dexonsmith, rafael,
>> >
>> > This patch modifies the memory buffer creation in the AsmParser library so that is requires a terminating null character.
>> > This change is needed because LLLexer in AsmParser checks for EOF when it sees the null character,
>> 
>> Is this a bug?  Should LLexer be fixed instead?
> 
> 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. 

Well, mine was just a gut reaction.  If Matthias is okay with it,
I am too.

@Alex, have a look -- if you think it'll add "a lot" of range checks
to LLexer, then I'm fine with the patch mostly as is.  But if it's
pretty simple then we might as well support `StringRef` fully.

If we go with your original patch, I have a comment on the test.

> Index: unittests/AsmParser/AsmParserTest.cpp
> ===================================================================
> --- /dev/null
> +++ unittests/AsmParser/AsmParserTest.cpp
> @@ -0,0 +1,37 @@
> +//===- llvm/unittest/AsmParser/AsmParserTest.cpp - asm parser unittests ---===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +
> +#include "llvm/ADT/StringRef.h"
> +#include "llvm/AsmParser/Parser.h"
> +#include "llvm/IR/LLVMContext.h"
> +#include "llvm/IR/Module.h"
> +#include "llvm/Support/SourceMgr.h"
> +#include "gtest/gtest.h"
> +
> +using namespace llvm;
> +
> +namespace {
> +
> +#ifdef GTEST_HAS_DEATH_TEST
> +#ifndef NDEBUG
> +
> +TEST(AsmParserTest, NonNullTerminatedInput) {
> +  LLVMContext &Ctx = getGlobalContext();
> +  StringRef Source = "; Empty module \n\1\2";
> +  SMDiagnostic Error;
> +  std::unique_ptr<Module> Mod;
> +  EXPECT_DEATH(Mod = parseAssemblyString(Source.substr(0, Source.size() - 2),
> +                                         Error, Ctx),
> +               "Buffer is not null terminated!");
> +}
> +
> +#endif
> +#endif

I think you should add another test that always runs, which has a
properly null-terminated comment and checks for sane results.

> +
> +} // end anonymous namespace