Object: Find terminator correctly when getting long filenames from GNU archives

[hans]

Summary

The code was previously assuming relying on there being a null terminator somewhere in (or after) the string table, something made less likely by r330786.

(Suggestions for how to test this welcome.)

[hans]

+pcc too

[ruiu]

Maybe you can test this by checking in a binary file?

[smeenai]

Is this fixing the same issue as D46214?

[inglorion]

Accepted so that you can land the fix if still needed, per my comment at https://bugs.chromium.org/p/chromium/issues/detail?id=840260#c9

[hans]

In D46527#1089776, @ruiu wrote:

Maybe you can test this by checking in a binary file?

I made an input file exactly 4096 bytes long and without any null bytes after the string, but I was not able to make it reliably crash :-(

In D46527#1089820, @smeenai wrote:

Is this fixing the same issue as D46214?

Yes, looks like exactly the same thing. I hadn't seen that one, and it seems my patch has more velocity now.

In D46527#1091014, @inglorion wrote:

Accepted so that you can land the fix if still needed, per my comment at https://bugs.chromium.org/p/chromium/issues/detail?id=840260#c9

Thanks! Yes, it's still needed. Our buildbot is as red as ever.

It's not ideal not having a test for this, but I still believe it's strictly an improvement so I'll go ahead and commit.

[pcc]

I made an input file exactly 4096 bytes long and without any null bytes after the string, but I was not able to make it reliably crash :-(

It could be that your file was not being memory mapped. See: http://llvm-cs.pcc.me.uk/lib/Support/MemoryBuffer.cpp#323

[inglorion]

Thanks, hans. I spent all of yesterday trying to cook up a test case for this. Taking into account the code in MemoryBuffer, I created a file of 20480 (4096 * 5) bytes to get it mmapped and a multiple of the page size, but it did not reproduce the problem. Just in case, I also tried a file not a multiple of the page size, and a small file. No luck. I tried just building the targets that were failing on the bot, but they built just fine locally. I tried using the exact Clang and Chromium revisions that the bot used, but, again, things worked fine locally. I tried the revisions I used a week ago, when I was able to get a repro, but no luck with that either. I also installed afl to try and fuzz my way to something that exercises the failure mode. I'll run that today and see if that gets me something we can use as a test case. At least we have the fix in now.

[inglorion]

Digging a bit deeper into this, I don't understand how this breaks. The crash indicates that we're running off the end of the file while reading from the long file name table while looking for a newline that isn't there. However, we seem to always create archives with the long file name table before any member that references it, and both GNU ar and llvm-ar will reject archives that are not structured that way. This means that there must be at least one member after the long file name table. Since the record at the beginning of a member ends in a newline (see ArchiveWriter::printRestOfMemberHeader), this means there must always be a newline between the end of the long file name table and the end of the file. For archives we write, every long file name including the last is followed by a newline (see ArchiveWriter::addToStringTable), so the newline we find should always be in memory we can read and always be at the end of the long file name we're looking at.

@hans, do you happen to have an archive that causes the crash without this fix having been applied? If so, I can take a look at that and see how I can turn it into a test case. If not, I'll just be happy that the fix is in and move on.

[pcc]

The issue is that the StringRef ctor was looking for a \0, not a /, so any / bytes in the file are irrelevant. Most object files will contain a \0 in their contents, which would terminate the search, but if the archive is a thin archive (as it is in this case), the file contents will not appear in the archive and the entire archive index will not contain any \0 bytes.

Here is a script that will create an archive that will reproduce the problem. You can also replace cru with cruT to create a thin archive with the same problem. I can only repro the crash on Windows; on my Linux machine an r/w page is mapped immediately after the archive file.

touch 012345678901234567890123456789012345678901234567890123456789012-{1..124}.o xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.o
rm -f file.a
../src2/llvm-project5/ra/bin/llvm-ar cru file.a 012345678901234567890123456789012345678901234567890123456789012-{1..124}.o xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx.o

[hans]

Thanks, Peter! I've committed a test based on this in r331854.

Bob: Sorry, I should have made the commit message a bit clearer. It mentioned looking for a null-byte, but it's not obvious that it's the StringRef constructor that was doing it, due to calling strlen. Lack of null-terminator is a classic bug when constructing StringRefs.

llvm/trunk/lib/Object/Archive.cpp

if (Name[0] == '/') {
     // Verify it.
     if (StringOffset >= Parent->getStringTable().size()) {
       uint64_t ArchiveOffset = reinterpret_cast<const char *>(ArMemHdr) -
                                Parent->getData().data();
       return malformedError("long name offset " + Twine(StringOffset) + " past "
                             "the end of the string table for archive member "
                             "header at offset " + Twine(ArchiveOffset));
     }
-    const char *addr = Parent->getStringTable().begin() + StringOffset;
 
     // GNU long file names end with a "/\n".
     if (Parent->kind() == Archive::K_GNU ||
         Parent->kind() == Archive::K_GNU64) {
-      StringRef::size_type End = StringRef(addr).find('\n');
-      return StringRef(addr, End - 1);
+      size_t End = Parent->getStringTable().find('\n', /*From=*/StringOffset);
+      if (End == StringRef::npos || End < 1 ||
+          Parent->getStringTable()[End - 1] != '/') {
+        return malformedError("string table at long name offset " +
+                              Twine(StringOffset) + "not terminated");
       }
-    return addr;
+      return Parent->getStringTable().slice(StringOffset, End - 1);
+    }
+    return Parent->getStringTable().begin() + StringOffset;
   }
 
   if (Name.startswith("#1/")) {
     uint64_t NameLength;
     if (Name.substr(3).rtrim(' ').getAsInteger(10, NameLength)) {
       std::string Buf;
       raw_string_ostream OS(Buf);
       OS.write_escaped(Name.substr(3).rtrim(' '));