[PATCH] D75742: [Object] Support large archive members

[George Rimar via Phabricator via llvm-commits]

grimar accepted this revision.
grimar added a comment.

LGTM. Using unit test is good I think.

In D75742#1912125 <https://reviews.llvm.org/D75742#1912125>, @jhenderson wrote:

> In D75742#1911048 <https://reviews.llvm.org/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.



================
Comment at: llvm/unittests/Object/ArchiveTest.cpp:25
+
+static char const ThinArchiveWithMember[] = "!<thin>\n"        // Global header
+                                            "test/           " // Member name
----------------
`static char const` -> `static const char`?


================
Comment at: llvm/unittests/Object/ArchiveTest.cpp:66
+  auto Child = (*A)->child_begin(ChildErr);
+  ASSERT_THAT_ERROR(std::move(ChildErr), Succeeded());
+
----------------
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`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75742/new/

https://reviews.llvm.org/D75742