<div dir="ltr">I see -- stringRef needs to be used with more care.<div><br></div><div>In this case, is it better to open the buffer in the caller of parseInputFilenamesFile? Passing unique_ptr around does not make that clear especially that the returned Buf looks like not used anywhere (but just to extends its lifetime).</div><div><br></div><div>David</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jun 3, 2016 at 4:46 PM, Vedant Kumar <span dir="ltr"><<a href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The WeightedFile instances store StringRefs into the MemoryBuffer backing InputFilenamesFile.<br>
<br>
Previously, the lifetime of the MemoryBuffer ended before we returned from parseInputFilenamesFile().<br>
<br>
With this commit we extend the lifetime of the MemoryBuffer, so the WeightedFile instances are always valid.<br>
<span class="HOEnZb"><font color="#888888"><br>
vedant<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> On Jun 3, 2016, at 4:44 PM, Xinliang David Li <<a href="mailto:xinliangli@gmail.com">xinliangli@gmail.com</a>> wrote:<br>
><br>
> This seems subtle. Can you explain a little more?<br>
><br>
> David<br>
><br>
> On Fri, Jun 3, 2016 at 4:12 PM, Vedant Kumar via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: vedantk<br>
> Date: Fri Jun  3 18:12:38 2016<br>
> New Revision: 271748<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=271748&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=271748&view=rev</a><br>
> Log:<br>
> [llvm-profdata] Fix use-after-free from discarded MemoryBuffer (NFC)<br>
><br>
> Thanks to Justin Bogner for pointing this out!<br>
><br>
> Caught by ASAN.<br>
><br>
> Modified:<br>
>     llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp<br>
><br>
> Modified: llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp?rev=271748&r1=271747&r2=271748&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp?rev=271748&r1=271747&r2=271748&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp (original)<br>
> +++ llvm/trunk/tools/llvm-profdata/llvm-profdata.cpp Fri Jun  3 18:12:38 2016<br>
> @@ -223,16 +223,19 @@ static WeightedFile parseWeightedFile(co<br>
>    return WeightedFile(FileName, Weight);<br>
>  }<br>
><br>
> -static void parseInputFilenamesFile(const StringRef &InputFilenamesFile,<br>
> -                                    WeightedFileVector &WFV) {<br>
> +static std::unique_ptr<MemoryBuffer><br>
> +parseInputFilenamesFile(const StringRef &InputFilenamesFile,<br>
> +                        WeightedFileVector &WFV) {<br>
>    if (InputFilenamesFile == "")<br>
> -    return;<br>
> +    return {};<br>
><br>
> -  auto Buf = MemoryBuffer::getFileOrSTDIN(InputFilenamesFile);<br>
> -  if (!Buf)<br>
> -    exitWithErrorCode(Buf.getError(), InputFilenamesFile);<br>
> +  auto BufOrError = MemoryBuffer::getFileOrSTDIN(InputFilenamesFile);<br>
> +  if (!BufOrError)<br>
> +    exitWithErrorCode(BufOrError.getError(), InputFilenamesFile);<br>
> +<br>
> +  std::unique_ptr<MemoryBuffer> Buffer = std::move(*BufOrError);<br>
> +  StringRef Data = Buffer->getBuffer();<br>
><br>
> -  StringRef Data = Buf.get()->getBuffer();<br>
>    SmallVector<StringRef, 8> Entries;<br>
>    Data.split(Entries, '\n', /*MaxSplit=*/-1, /*KeepEmpty=*/false);<br>
>    for (const StringRef &FileWeightEntry : Entries) {<br>
> @@ -246,6 +249,8 @@ static void parseInputFilenamesFile(cons<br>
>      else<br>
>        WFV.emplace_back(parseWeightedFile(SanitizedEntry));<br>
>    }<br>
> +<br>
> +  return Buffer;<br>
>  }<br>
><br>
>  static int merge_main(int argc, const char *argv[]) {<br>
> @@ -288,7 +293,7 @@ static int merge_main(int argc, const ch<br>
>      WeightedInputs.push_back(WeightedFile(Filename, 1));<br>
>    for (StringRef WeightedFilename : WeightedInputFilenames)<br>
>      WeightedInputs.push_back(parseWeightedFile(WeightedFilename));<br>
> -  parseInputFilenamesFile(InputFilenamesFile, WeightedInputs);<br>
> +  auto Buf = parseInputFilenamesFile(InputFilenamesFile, WeightedInputs);<br>
><br>
>    if (WeightedInputs.empty())<br>
>      exitWithError("No input files specified. See " +<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div></blockquote></div><br></div>