[LLD][COFF] Report error when file will exceed Windows maximum image size (4GB)

[colden]

Summary

Currently, when a large PE (>4 GiB) is to be produced, a crash occurs because:

1. Calling setOffset with a number greater than UINT32_MAX causes the PointerToRawData to overflow
2. When adding the symbol table to the end of the file, the last section's offset was used to calculate file size. Because this had overflowed, this number was too low, and the file created would not be large enough. This lead to the actual crash I saw, which was a buffer overrun.

This change:

1. Adds comment to setOffset, clarifying that overflow can occur, but it's somewhat safe because the error will be handled elsewhere
2. Adds file size check after all output data has been created This matches the MS link.exe error, which looks prints as: "LINK : fatal error LNK1248: image size (10000EFC9) exceeds maximum allowable size (FFFFFFFF)"
3. Changes calculate of the symbol table offset to just use the existing FileSize. This should match the previous calculations, but doesn't rely on the use of a u32 that can overflow.
4. Removes trivial usage of a magic number that bugged me while I was debugging the issue

I'm not sure how to add a test for this outside of adding 4GB of object files to the repo. If there's an easier way, let me know and I'll be happy to add a test.

[ruiu]

Generally looking fine.

tools/lld/COFF/Writer.cpp

302 ↗

(On Diff #129522)

I feel printing out a file size in hexadecimal is somewhat unusual. Maybe decimal is better?

[colden]

tools/lld/COFF/Writer.cpp

302 ↗

(On Diff #129522)

Yeah, I was kind of surprised to see the MS linker does hex. I think you're right though, I'll switch it back.

[colden]

Switch error message to use decimal format from hex.

[ruiu]

tools/lld/COFF/Writer.cpp

303–305 ↗

(On Diff #129729)

I believe you can do

fatal("image size ("+ Twine(FileSize) + ") exceeds maximum allowable size (" + Twine(UINT32_MAX) + ")");

[colden]

Turns out you can do the easier thing

[ruiu]

LGTM. Thanks!

[colden]

Thanks for the quick reviews!

Would you mind committing this for me?

[colden]

Bump, could someone submit this for me?

[rnk]

tools/lld/COFF/Writer.cpp

303–305 ↗

(On Diff #129729)

Apologies in advance for nit picking, but why not say something more human readable like "4GB"?

[colden]

tools/lld/COFF/Writer.cpp

303–305 ↗

(On Diff #129729)

No problem, I'm happy to entertain nits :)

If we do this, I think we should represent the current size in GB as well. Printing two numbers that are not directly comparable would drive me nuts as a user. Is there an easy conversion function for this somewhere, or should I just static_cast to float and divide by 1073741824?

[rnk]

tools/lld/COFF/Writer.cpp

303–305 ↗

(On Diff #129729)

Nevermind, I think comparability is a great reason to keep this as it is.

lld/trunk/COFF/Writer.cpp

 }
 
 void OutputSection::setFileOffset(uint64_t Off) {
   // If a section has no actual data (i.e. BSS section), we want to
   // set 0 to its PointerToRawData. Otherwise the output is rejected
   // by the loader.
   if (Header.SizeOfRawData == 0)
     return;
+
+  // It is possible that this assignment could cause an overflow of the u32,
+  // but that should be caught by the FileSize check in OutputSection::run().
   Header.PointerToRawData = Off;
 }
 
 void OutputSection::addChunk(Chunk *C) {
   Chunks.push_back(C);
   C->setOutputSection(this);
   uint64_t Off = Header.VirtualSize;
   Off = alignTo(Off, C->Alignment);
void Writer::run() {
   createExportTable();
   if (Config->Relocatable)
     createSection(".reloc");
   assignAddresses();
   removeEmptySections();
   setSectionPermissions();
   createSymbolAndStringTable();
 
+  if (FileSize > UINT32_MAX)
+    fatal("image size (" + Twine(FileSize) + ") " +
+        "exceeds maximum allowable size (" + Twine(UINT32_MAX) + ")");
+
   // We must do this before opening the output file, as it depends on being able
   // to read the contents of the existing output file.
   PreviousBuildId = loadExistingBuildId(Config->OutputFile);
   openFile(Config->OutputFile);
   if (Config->is64()) {
     writeHeader<pe32plus_header>();
   } else {
     writeHeader<pe32_header>();
for (ObjFile *File : ObjFile::Instances) {
           OutputSymtab.push_back(*Sym);
       }
     }
   }
 
   if (OutputSymtab.empty() && Strtab.empty())
     return;
 
-  OutputSection *LastSection = OutputSections.back();
   // We position the symbol table to be adjacent to the end of the last section.
-  uint64_t FileOff = LastSection->getFileOff() +
-                     alignTo(LastSection->getRawSize(), SectorSize);
+  uint64_t FileOff = FileSize;
   PointerToSymbolTable = FileOff;
   FileOff += OutputSymtab.size() * sizeof(coff_symbol16);
   FileOff += 4 + Strtab.size();
   FileSize = alignTo(FileOff, SectorSize);
 }
 
 // Visits all sections to assign incremental, non-overlapping RVAs and
 // file offsets.
 void Writer::assignAddresses() {
   SizeOfHeaders = DOSStubSize + sizeof(PEMagic) + sizeof(coff_file_header) +
                   sizeof(data_directory) * NumberfOfDataDirectory +
                   sizeof(coff_section) * OutputSections.size();
   SizeOfHeaders +=
       Config->is64() ? sizeof(pe32plus_header) : sizeof(pe32_header);
   SizeOfHeaders = alignTo(SizeOfHeaders, SectorSize);
-  uint64_t RVA = 0x1000; // The first page is kept unmapped.
+  uint64_t RVA = PageSize; // The first page is kept unmapped.
   FileSize = SizeOfHeaders;
   // Move DISCARDABLE (or non-memory-mapped) sections to the end of file because
   // the loader cannot handle holes.
   std::stable_partition(
       OutputSections.begin(), OutputSections.end(), [](OutputSection *S) {
         return (S->getPermissions() & IMAGE_SCN_MEM_DISCARDABLE) == 0;
       });
   for (OutputSection *Sec : OutputSections) {