[memprof] Fix use-after-free in peekBuildIds.

[snehasish]

Summary

To check the uniqueness of buildids, we held on to a StringRef of the build id string pushed into the vector. If the number of build ids were large enough to trigger a realloc in the vector then these references where invalidated resulting in a use-after free. This was exposed in downstream usage.

[tejohnson]

lgtm but you could also consider collapsing both BuildIds and BuildIdsSet into a single SetVector for conciseness.

[snehasish]

In D155110#4495040, @tejohnson wrote:

lgtm but you could also consider collapsing both BuildIds and BuildIdsSet into a single SetVector for conciseness.

I did consider this prior to sending out the patch. Since SetVector::takeVector return a SmallVector we need to either

• create a new result std::vector prior to avoid changing the return type, negating the benefit
• update the return type which breaks downstream usage and will need to be updated

Since this isn't performance critical I chose the simplest approach.

[tejohnson]

In D155110#4495062, @snehasish wrote:

In D155110#4495040, @tejohnson wrote:

lgtm but you could also consider collapsing both BuildIds and BuildIdsSet into a single SetVector for conciseness.

I did consider this prior to sending out the patch. Since SetVector::takeVector return a SmallVector we need to either

• create a new result std::vector prior to avoid changing the return type, negating the benefit
• update the return type which breaks downstream usage and will need to be updated

Since this isn't performance critical I chose the simplest approach.

Ack - thanks for the explanation. Agree with the approach you chose.

llvm/lib/ProfileData/RawMemProfReader.cpp

RawMemProfReader::peekBuildIds(MemoryBuffer *DataBuffer) {
   const char *Next = DataBuffer->getBufferStart();
   // Use a set + vector since a profile file may contain multiple raw profile
   // dumps, each with segment information. We want them unique and in order they
   // were stored in the profile; the profiled binary should be the first entry.
   // The runtime uses dl_iterate_phdr and the "... first object visited by
   // callback is the main program."
   // https://man7.org/linux/man-pages/man3/dl_iterate_phdr.3.html
   std::vector<std::string> BuildIds;
-  llvm::SmallSet<StringRef, 4> BuildIdsSet;
+  llvm::SmallSet<std::string, 10> BuildIdsSet;
   while (Next < DataBuffer->getBufferEnd()) {
     auto *Header = reinterpret_cast<const memprof::Header *>(Next);
 
     const llvm::SmallVector<SegmentEntry> Entries =
         readSegmentEntries(Next + Header->SegmentOffset);
 
     for (const auto &Entry : Entries) {
       const std::string Id = getBuildIdString(Entry);
       if (BuildIdsSet.contains(Id))
         continue;
       BuildIds.push_back(Id);
-      BuildIdsSet.insert(BuildIds.back());
+      BuildIdsSet.insert(Id);
     }
 
     Next += Header->TotalSize;
   }
   return BuildIds;
 }
 
 Error RawMemProfReader::readRawProfile(