This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by hans on May 7 2018, 8:04 AM.

Details

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

Diff Detail

Event Timeline

hans created this revision.May 7 2018, 8:04 AM
hans added a reviewer: pcc.May 7 2018, 8:20 AM

+pcc too

ruiu added a comment.May 7 2018, 8:27 AM

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

smeenai added a subscriber: smeenai.May 7 2018, 8:57 AM

Is this fixing the same issue as D46214?

inglorion accepted this revision.May 8 2018, 12:55 AM

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

This revision is now accepted and ready to land.May 8 2018, 12:55 AM
hans added a comment.May 8 2018, 1:26 AM

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 :-(

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.

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.

This revision was automatically updated to reflect the committed changes.
pcc added a comment.May 8 2018, 9:05 AM

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

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.

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 added a comment.May 8 2018, 6:27 PM

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 added a comment.May 9 2018, 1:25 AM

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.