[Object] Support large archive members

[jhenderson]

Summary

The archive library truncated the size of archive members whose size was greater than max uint32_t, leading to errors in tools that use the library. This patch fixes the issue and adds some unit tests to verify.

[MaskRay]

Command Output (stderr):
--

bzip2: Compressed file ends unexpectedly;
	perhaps it is corrupted?  *Possible* reason follows.
bzip2: Success
	Input file = /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/lld/test/ELF/Inputs/large-files1.o.bz2, output file = (stdout)

It is possible that the compressed file(s) have become corrupted.
You can use the -tvv option to test integrity of such files.

You can use the `bzip2recover' program to attempt to recover
data from undamaged sections of corrupted files.

As you see, many systems may not be able to run such tests. Deleting the test may be fine.

llvm/lib/Object/Archive.cpp

394–395

Does return Header.getSize() work?

[grimar]

I am also not fond of having tests that might consume gigabytes to run.
Perhaps you could craft an object that has a broken size written to a header, e.g. (MAX_UINT32 + 1) ant test
that LLD does not crash and report an error with that would contain the size value found?

[jhenderson]

In D75742#1911048, @grimar wrote:

Perhaps you could craft an object that has a broken size written to a header, e.g. (MAX_UINT32 + 1) ant test
that LLD does not crash and report an error with that would contain the size value found?

I'm not sure I understand what you mean here. The bug fixed by this patch manifests as attempting to read a section header table past the end of the file, because the result of the truncated size is failing to create a buffer that is large enough. This fix can therefore only be verified by attempting to use data in the latter part of a buffer that is > 4GB in size. The only alternative I can think of to having large files as inputs, is to create sparse files, but I'm not sure if the test would be portable consequently.

[jhenderson]

Simplify size handling slightly.

I've not deleted the test yet, but I will do so in due course before this gets committed. I'd like to see if we can come up with an alternative test option first, if we can. For the record, the test passes locally with my patch, but didn't without the fix (it was how I found the bug in the first place).

[jhenderson]

I realised that I could at least test Archive::Child::getSize() function using gtest unittests. I'll take a look at that, and see if getBuffer is possible too.

[jhenderson]

Test with unit tests.

[ruiu]

LGTM

[grimar]

LGTM. Using unit test is good I think.

In D75742#1912125, @jhenderson wrote:

In D75742#1911048, @grimar wrote:

Perhaps you could craft an object that has a broken size written to a header, e.g. (MAX_UINT32 + 1) ant test
that LLD does not crash and report an error with that would contain the size value found?

I'm not sure I understand what you mean here. The bug fixed by this patch manifests as attempting to read a section header table past the end of the file, because the result of the truncated size is failing to create a buffer that is large enough. This fix can therefore only be verified by attempting to use data in the latter part of a buffer that is > 4GB in size. The only alternative I can think of to having large files as inputs, is to create sparse files, but I'm not sure if the test would be portable consequently.

Idea was based on a testing of a values reported in a error message.
See the code this patch changes:

Expected<StringRef> Archive::Child::getBuffer() const {
  Expected<bool> isThinOrErr = isThinMember();
  if (!isThinOrErr)
    return isThinOrErr.takeError();
  bool isThin = isThinOrErr.get();
  if (!isThin) {
    Expected<uint64_t> Size = getSize();
    if (!Size)
      return Size.takeError();
    return StringRef(Data.data() + StartOfFile, Size.get());
  }

The problem is with the original logic after Expected<uint64_t> Size = getSize();.
Imagine we have a broken object and getSize() returns (uint64)-1.
We do not report it and return a StringRef that has overruns the file size.
But we could report an error (i.e. add a new one) instead.

This error could contain an information about a size of a broken child.
It would be printed differently in Expected<uint64_t> Size and Expected<uint32_t> Size
cases I think. That is what something I was thinking about.

llvm/unittests/Object/ArchiveTest.cpp

26

static char const -> static const char?

67

Not strong suggestion:

MemoryBufferRef Source(ArchiveWithMember, "regular");
Expected<std::unique_ptr<Archive>> A = Archive::create(Source);
ASSERT_THAT_EXPECTED(A, Succeeded());
Error ChildErr = Error::success();
auto Child = (*A)->child_begin(ChildErr);
ASSERT_THAT_ERROR(std::move(ChildErr), Succeeded());

This part is almost similar to one in ArchiveChildGetSizeThinArchive and ArchiveChildGetSizeRegularArchive.
Should this code be moved to a helper that returns a Child?

[jhenderson]

llvm/unittests/Object/ArchiveTest.cpp

67

I considered that. Unfortunately, ASSERT_* macros return void, so can't be inside other functions that return something else. Consequently, we have to put them in yet another function, then check to see if the thing is valid anyway (rendering the ASSERT pointless), and the logic becomes somewhat complex and hard to follow. I don't think there's much benefit as a result.

[rupprecht]

In D75742#1912140, @jhenderson wrote:

Simplify size handling slightly.

I've not deleted the test yet, but I will do so in due course before this gets committed. I'd like to see if we can come up with an alternative test option first, if we can. For the record, the test passes locally with my patch, but didn't without the fix (it was how I found the bug in the first place).

I don't know if it's portable (you'd have to add requires for Linux/shell, or maybe do some lit configuration to detect if it's available), but you could try using fallocate:

$ time fallocate -l 3G test.large
fallocate -l 3G test.large  0.00s user 0.01s system 39% cpu 0.017 total
$ time cat test.large > /dev/null
cat test.large > /dev/null  0.01s user 1.18s system 99% cpu 1.193 total

If you want to try that, I would still recommend keeping the unit test that runs on all platforms, and just having this lit one be an extra test.

llvm/unittests/Object/ArchiveTest.cpp

67

One way you might be able to refactor this is to not have the assert in the helper method:

Expected<Child> createChild(StringRef src, StringRef name) {
  MemoryBufferRef Source(src, name);
  Expected<std::unique_ptr<Archive>> A = Archive::create(Source);
  if (!A) return A.takeError();

  Error ChildErr = Error::success();
  auto Child = (*A)->child_begin(ChildErr);
  if (ChildErr) return std::move(ChildErr);
  return std::move(Child);
}

TEST(ArchiveTests, ArchiveChildGetBuffer) {
  auto Child = createChild(ArchiveWithMember, "regular");
  ASSERT_THAT_EXPECTED(Child, Succeeded());
...

(heavy pseudocode, I almost definitely got a lot of types wrong... and I'm also ignoring the lifetime issues)

72

ASSERT_TRUE((bool)Buffer);

[jhenderson]

In D75742#1914799, @rupprecht wrote:

In D75742#1912140, @jhenderson wrote:

Simplify size handling slightly.

I've not deleted the test yet, but I will do so in due course before this gets committed. I'd like to see if we can come up with an alternative test option first, if we can. For the record, the test passes locally with my patch, but didn't without the fix (it was how I found the bug in the first place).

I don't know if it's portable (you'd have to add requires for Linux/shell, or maybe do some lit configuration to detect if it's available), but you could try using fallocate:

$ time fallocate -l 3G test.large
fallocate -l 3G test.large  0.00s user 0.01s system 39% cpu 0.017 total
$ time cat test.large > /dev/null
cat test.large > /dev/null  0.01s user 1.18s system 99% cpu 1.193 total

If you want to try that, I would still recommend keeping the unit test that runs on all platforms, and just having this lit one be an extra test.

I'm not sure that's going to work. We'll still need an object of the right size in the first place, to punch holes in, as otherwise it won't be a valid linker input, so it doesn't avoid the size issue. Furthermore, I doubt that llvm-ar or LLD will preserve the sparseness, so it won't be useful for testing this or LLD's behaviour.

[jhenderson]

Address review comments (char const -> const char, pull code out into helper).

[MaskRay]

In D75742#1914799, @rupprecht wrote:

In D75742#1912140, @jhenderson wrote:

Simplify size handling slightly.

I've not deleted the test yet, but I will do so in due course before this gets committed. I'd like to see if we can come up with an alternative test option first, if we can. For the record, the test passes locally with my patch, but didn't without the fix (it was how I found the bug in the first place).

I don't know if it's portable (you'd have to add requires for Linux/shell, or maybe do some lit configuration to detect if it's available), but you could try using fallocate:

$ time fallocate -l 3G test.large
fallocate -l 3G test.large  0.00s user 0.01s system 39% cpu 0.017 total
$ time cat test.large > /dev/null
cat test.large > /dev/null  0.01s user 1.18s system 99% cpu 1.193 total

If you want to try that, I would still recommend keeping the unit test that runs on all platforms, and just having this lit one be an extra test.

Nice tip! Sparse files is even supported on ext2. So running it on Linux will be safe. However, if the disk cannot allocate 3G space (if the space is less 3G, then it surely cannot), fallocate(2) can fail with ENOSPC 28 No space left on device.

Additionally, if a test can take more than one second, I will still be hesitant..

ld.lld -m elf_x86_64 -b binary test.large -o out can take quite a few seconds. 2.5s in tmpfs with warn cache on my pc. It could be much slower with a different configuration.

[grimar]

FTR.

The current size of LLVM build folder for me is 26.1 Gb.
I think it is reasonable to avoid letting it go to 30.1 Gb (e.g. +4 Gb, +15%).

We have huge-temporarily-file.s test in LLD with the following comment:

1. This inputs previously created a 4gb temporarily file under 32 bit
2. configuration. Issue was fixed. There is no clean way to check that from here.
3. This testcase added for documentation purposes.

I believe that +15% of peak free space consumption is probably too much. It might be better to
have no test at all sometimes.

[jhenderson]

In D75742#1916287, @grimar wrote:

FTR.

The current size of LLVM build folder for me is 26.1 Gb.
I think it is reasonable to avoid letting it go to 30.1 Gb (e.g. +4 Gb, +15%).

We have huge-temporarily-file.s test in LLD with the following comment:

1. This inputs previously created a 4gb temporarily file under 32 bit
2. configuration. Issue was fixed. There is no clean way to check that from here.
3. This testcase added for documentation purposes.

I believe that +15% of peak free space consumption is probably too much. It might be better to
have no test at all sometimes.

We have or had that file? I couldn't find it in git as things stand.

[grimar]

In D75742#1916519, @jhenderson wrote:

In D75742#1916287, @grimar wrote:

FTR.

The current size of LLVM build folder for me is 26.1 Gb.
I think it is reasonable to avoid letting it go to 30.1 Gb (e.g. +4 Gb, +15%).

We have huge-temporarily-file.s test in LLD with the following comment:

1. This inputs previously created a 4gb temporarily file under 32 bit
2. configuration. Issue was fixed. There is no clean way to check that from here.
3. This testcase added for documentation purposes.

I believe that +15% of peak free space consumption is probably too much. It might be better to
have no test at all sometimes.

We have or had that file? I couldn't find it in git as things stand.

I believe we have it:
https://github.com/llvm/llvm-project/blob/master/lld/test/ELF/linkerscript/huge-temporary-file.s

[jhenderson]

In D75742#1916549, @grimar wrote:

In D75742#1916519, @jhenderson wrote:

In D75742#1916287, @grimar wrote:

FTR.

The current size of LLVM build folder for me is 26.1 Gb.
I think it is reasonable to avoid letting it go to 30.1 Gb (e.g. +4 Gb, +15%).

We have huge-temporarily-file.s test in LLD with the following comment:

1. This inputs previously created a 4gb temporarily file under 32 bit
2. configuration. Issue was fixed. There is no clean way to check that from here.
3. This testcase added for documentation purposes.

I believe that +15% of peak free space consumption is probably too much. It might be better to
have no test at all sometimes.

We have or had that file? I couldn't find it in git as things stand.

I believe we have it:
https://github.com/llvm/llvm-project/blob/master/lld/test/ELF/linkerscript/huge-temporary-file.s

Thanks. I didn't look in the linkerscript directory.

llvm/lib/Object/Archive.cpp

if (Name.substr(3).rtrim(' ').getAsInteger(10, NameSize)) {
                             "archive member header at offset " +
                             Twine(Offset));
       return;
     }
     StartOfFile += NameSize;
   }
 }
 
 Expected<uint64_t> Archive::Child::getSize() const {
-  if (Parent->IsThin) {
-    Expected<uint32_t> Size = Header.getSize();
-    if (!Size)
-      return Size.takeError();
-    return Size.get();
-  }
+  if (Parent->IsThin)
+    return Header.getSize();
   return Data.size() - StartOfFile;
 }
 
 Expected<uint64_t> Archive::Child::getRawSize() const {
   return Header.getSize();
 }
 
 Expected<bool> Archive::Child::isThinMember() const {

 }
 
 Expected<StringRef> Archive::Child::getBuffer() const {
   Expected<bool> isThinOrErr = isThinMember();
   if (!isThinOrErr)
     return isThinOrErr.takeError();
   bool isThin = isThinOrErr.get();
   if (!isThin) {
-    Expected<uint32_t> Size = getSize();
+    Expected<uint64_t> Size = getSize();
     if (!Size)
       return Size.takeError();
     return StringRef(Data.data() + StartOfFile, Size.get());
   }
   Expected<std::string> FullNameOrErr = getFullName();
   if (!FullNameOrErr)
     return FullNameOrErr.takeError();
   const std::string &FullName = *FullNameOrErr;

llvm/unittests/Object/ArchiveTest.cpp

+//===- ArchiveTest.cpp ----------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Object/Archive.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace object;
+using namespace testing;
+
+static const char ArchiveWithMember[] = "!<arch>\n"        // Global header
+                                        "test/           " // Member name
+                                        "0           "     // Timestamp
+                                        "0     "           // Owner ID
+                                        "0     "           // Group ID
+                                        "0       "         // File mode
+                                        "9999999999"       // Size
+                                        "`\n";
+
+static const char ThinArchiveWithMember[] = "!<thin>\n"        // Global header
+                                            "test/           " // Member name
+                                            "0           "     // Timestamp
+                                            "0     "           // Owner ID
+                                            "0     "           // Group ID
+                                            "0       "         // File mode
+                                            "9999999999"       // Size
+                                            "`\n";
+
+struct ArchiveTestsFixture : Test {
+  Expected<Archive::child_iterator> createChild(StringRef Src) {
+    MemoryBufferRef Source(Src, "archive");
+    Expected<std::unique_ptr<Archive>> AOrErr = Archive::create(Source);
+    if (!AOrErr)
+      return AOrErr.takeError();
+    A = std::move(*AOrErr);
+
+    Error ChildErr = Error::success();
+    auto Child = A->child_begin(ChildErr);
+    if (ChildErr)
+      return std::move(ChildErr);
+    return Child;
+  }
+
+  std::unique_ptr<Archive> A;
+};
+
+TEST_F(ArchiveTestsFixture, ArchiveChildGetSizeRegularArchive) {
+  auto Child = createChild(ArchiveWithMember);
+  ASSERT_THAT_EXPECTED(Child, Succeeded());
+
+  Expected<uint64_t> Size = (*Child)->getSize();
+  ASSERT_THAT_EXPECTED(Size, Succeeded());
+  EXPECT_EQ(9999999999, *Size);
+}
+
+TEST_F(ArchiveTestsFixture, ArchiveChildGetSizeThinArchive) {
+  auto Child = createChild(ThinArchiveWithMember);
+  ASSERT_THAT_EXPECTED(Child, Succeeded());
+
+  Expected<uint64_t> Size = (*Child)->getSize();
+  ASSERT_THAT_EXPECTED(Size, Succeeded());
+  EXPECT_EQ(9999999999, *Size);
+}
+
+TEST_F(ArchiveTestsFixture, ArchiveChildGetBuffer) {
+  auto Child = createChild(ArchiveWithMember);
+  ASSERT_THAT_EXPECTED(Child, Succeeded());
+
+  Expected<StringRef> Buffer = (*Child)->getBuffer();
+  // Cannot use ASSERT_THAT_EXPECTED, as that will attempt to print the
+  // StringRef (which has an invalid size).
+  ASSERT_TRUE((bool)Buffer);
+  EXPECT_EQ(9999999999, Buffer->size());
+  EXPECT_EQ(ArchiveWithMember + sizeof(ArchiveWithMember) - 1, Buffer->data());
+}

llvm/unittests/Object/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   BinaryFormat
   Object
   )
 
 add_llvm_unittest(ObjectTests
+  ArchiveTest.cpp
   MinidumpTest.cpp
   ObjectFileTest.cpp
   SymbolSizeTest.cpp
   SymbolicFileTest.cpp
   )
 
 target_link_libraries(ObjectTests PRIVATE LLVMTestingSupport)