[ELF] - Do not crash on large output.

[grimar]

Summary

I found at least 2 possible situations when we may crash on large outputs:

Imagine we have large section sizes. It is simulated in a testcase by providing alignment of 0xFFFFFFFF for 32 bit target.
Then overflow may happen during assigning offsets.

[ruiu]

ELF/Writer.cpp

1388–1389

You should consider the following four cases.

Host / Target
32/32 - This check will never fail and silently create broken file.
32/64 - This check will never fail but if you attempt to create a file larger than 4GB, the linker will crash at some point because we are using MMAP IO and the result won't fit in the memory space.
64/32 - This check can catch errors.
64/64 - This check will never fail and silently create broken files.

As you can see, it results in pretty inconsistent behavior.

If you really want to do something about it, please do this.

• Change FileSize's type to uint64_t
• and verify that FileSize < SIZE_MAX && FileSize < sizeof(uintX_t)*8.

[grimar]

ELF/Writer.cpp

1388–1389

Host / Target
32/32 - This check will never fail and silently create broken file.

Will never fail, but will never create broken file also I think. Unless you mean that overflow of FileSize can happen earlier ? That true and that is what I am trying to diagnose in setOffset(). Changing FileSize type to uint64_t probably can help here, but unfortunately not for 64/64 case below.

32/64 - This check will never fail but if you attempt to create a file larger than 4GB, the linker will crash at some point because we are using MMAP IO and the result won't fit in the memory space.

Agree.

64/32 - This check can catch errors.

Ok.

64/64 - This check will never fail and silently create broken files.

The same as 32/32 I think, but with one more detail.
Do I correctrly understand your idea that you want remove code from setOffset() and just do these 2 suggested changes ? If so I can imaging easy testcase that can break it: just x64 object with section alignment of 0xFFFFFFFFFFFFFFFF (similar to what I have for i386 now) will cause overflow in setOffset() and suggested check will not work :(

So do you think we should just ignore possible fail of 64/64 case and just stop on your suggested change ?

[grimar]

I`ll try to update this tomorrow. Found that VA also may overflow in next assignAddresses():

template <class ELFT> void Writer<ELFT>::assignAddresses() {
  uintX_t VA = Config->ImageBase + getHeaderSize<ELFT>();
...

And VA affects on how we do calculate offsets.

[• rafael]

ELF/Writer.cpp

1233

I wonder how many cases there can be where overflow and if there is a general solution.

How was this crashing before?

1388

Maybe move this to another patch?

test/ELF/invalid/too-large-output-i386.s

4

Can you create this with yaml2obj?

[grimar]

test/ELF/invalid/too-large-output-i386.s

4

Nope.
Actually I can, but result binary will be too large.

section-alignment.test creates file with big alignment:

Sections:
  - Name:            .text
    Type:            SHT_PROGBITS
    Flags:           [ SHF_ALLOC, SHF_EXECINSTR ]
    AddressAlign:    0x1000000000000001
    Content:         "00000000"

But that works fine only because AddressAlign is casted to unsigned inside yaml2obj and truncated to 0 finally. So produced binary is little.
But 0xFFFFFFFF value will produce huge binary, what probably not good for testcase.

I noticed this behavior today, not sure how much it is correct.

[grimar]

ELF/Writer.cpp

1233

Yes, that is a problem, solution is not general :( Crash was because of overflow of Off that is used to calculate FileSize.
So file created was little and it then crashes in writeTo during writing output sections.
We do not check end of buffer there. Probably it can be a that "general solution". I`ll try to do domething tomorrow with that.

[grimar]

• Addressed review comments.

[grimar]

• Restored testcase missing in latest diff.
• Improved error diagnostic messages.

[ruiu]

ELF/Writer.cpp

1165

This is I think just too much. We shouldn't introduce this integer-ish class just to check for overflow.

[grimar]

ELF/Writer.cpp

1165

Posted different approach: D25467

[grimar]

Depricated.

ELF/Writer.cpp

if (needsPtLoad(Sec)) {
       uintX_t TVA = VA + ThreadBssOffset;
       TVA = alignTo(TVA, Alignment);
       Sec->setVA(TVA);
       ThreadBssOffset = TVA - VA + Sec->getSize();
     }
   }
 }
 
+template <class T> class CheckedType {
+  T Val;
+  StringRef Err;
+
+public:
+  CheckedType(T Val, StringRef Err) : Val(Val), Err(Err){};
+
+  operator uint64_t() { return Val; }
+  operator uint32_t() {
+    if (Val > UINT32_MAX)
+      fatal(Err + "checked type operation overflow");
+    return Val;
+  }
+
+  template <class T2> CheckedType operator+(T2 R) {
+    if (R > getMax() || (this->Val > getMax() - R))
+      fatal(Err + "checked type operation overflow");
+    return {this->Val + R, Err};
+  }
+
+  template <class T2> CheckedType operator-(T2 R) {
+    if (R > this->Val)
+      fatal(Err + "checked type operation overflow");
+    return {this->Val - R, Err};
+  }
+
+private:
+  T getMax() { return std::numeric_limits<typename T>::max(); }
+};
+
+template <class T> CheckedType<T> makeChecked(T Val, StringRef S) {
+  return CheckedType<T>(Val, S);
+}
+
 // Adjusts the file alignment for a given output section and returns
 // its new file offset. The file offset must be the same with its
 // virtual address (modulo the page size) so that the loader can load
 // executables without any address adjustment.
 template <class ELFT, class uintX_t>
 static uintX_t getFileAlignment(uintX_t Off, OutputSectionBase<ELFT> *Sec) {
   uintX_t Alignment = Sec->getAlignment();
   if (Sec->PageAlign)
     Alignment = std::max<uintX_t>(Alignment, Config->MaxPageSize);
   Off = alignTo(Off, Alignment);
 
   OutputSectionBase<ELFT> *First = Sec->FirstInPtLoad;
   // If the section is not in a PT_LOAD, we have no other constraint.
   if (!First)
     return Off;
 
   // If two sections share the same PT_LOAD the file offset is calculated using
   // this formula: Off2 = Off1 + (VA2 - VA1).
   if (Sec == First)
     return alignTo(Off, Target->MaxPageSize, Sec->getVA());
-  return First->getFileOffset() + Sec->getVA() - First->getVA();
+  return makeChecked(First->getFileOffset(), "getFileAlignment failed: ") +
+         Sec->getVA() - First->getVA();
 }
 
 template <class ELFT, class uintX_t>
 void setOffset(OutputSectionBase<ELFT> *Sec, uintX_t &Off) {
   if (Sec->getType() == SHT_NOBITS) {
     Sec->setFileOffset(Off);
     return;
   }
 
   Off = getFileAlignment<ELFT>(Off, Sec);
   Sec->setFileOffset(Off);
-  Off += Sec->getSize();
+  Off = makeChecked(Off, "setOffset failed: ") + Sec->getSize();
 }
 
 template <class ELFT> void Writer<ELFT>::assignFileOffsetsBinary() {
   uintX_t Off = 0;
   for (OutputSectionBase<ELFT> *Sec : OutputSections)
     if (Sec->getFlags() & SHF_ALLOC)
       setOffset(Sec, Off);
   FileSize = alignTo(Off, sizeof(uintX_t));
 }
template <class ELFT> void Writer<ELFT>::writeHeader() {
 
   // Write the section header table. Note that the first table entry is null.
   auto *SHdrs = reinterpret_cast<Elf_Shdr *>(Buf + EHdr->e_shoff);
   for (OutputSectionBase<ELFT> *Sec : OutputSections)
     Sec->writeHeaderTo(++SHdrs);
 }
 
 template <class ELFT> void Writer<ELFT>::openFile() {
   ErrorOr<std::unique_ptr<FileOutputBuffer>> BufferOrErr =
       FileOutputBuffer::create(Config->OutputFile, FileSize,
                                FileOutputBuffer::F_executable);
   if (auto EC = BufferOrErr.getError())
     error(EC, "failed to open " + Config->OutputFile);
   else
     Buffer = std::move(*BufferOrErr);
 }
 
 template <class ELFT> void Writer<ELFT>::writeSectionsBinary() {

test/ELF/invalid/too-large-output-i386.s

+# REQUIRES: x86
+
+## too-large-output-i386.elf contains section with address align.
+## of 0xFFFFFFFF. Total file size calculation overflows because of that.
+# RUN: not ld.lld -shared %S/Inputs/too-large-output-i386.elf -o %t 2>&1 \
+# RUN:  | FileCheck %s
+# CHECK: checked type operation overflow