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

[Alex L]

2015-05-20 11:36 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:

>
> > 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.
>

It only adds one range check in the 'getNextChar' method. It's also
the only method that has to be modified. But I'm not sure what
performance impact it would have, although there's already a check for
a null character in that method.
I think I'll go ahead and submit a range check patch for now.

Alex.


>
> 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150520/aeb588f8/attachment.html>