[ELF] Check eh_frame_hdr overflow with PC offsets instead of PC absolute addresses

[ruiu]

ELF/SyntheticSections.cpp

510–513

I don't think this is a good place to check for this error. I'd move this to EhFrameHeader::writeTo(), so that getFdeData can just return FDE data without doing any extra work, as the name of that function implies.

513

Can't FdeVA overflow?

[MaskRay]

Add test

[MaskRay]

ELF/SyntheticSections.cpp

510–513

Then we would not know which object file causes .eh_frame_hdr pcrel overflow. But if you think this information is minor, I can move the check to EhFrameHeader::writeTo()

FdeVA is the address of the .eh_frame FDE entry. I think it is very unlikely to overflow

[ruiu]

As to FdeVA, I wanted to ask if it could theoretically overflow. Since the new FdeData struct takes 16 bytes anyway, there's no point to keep FdeVA uint32_t instead of uint64_t.

[ruiu]

Can you investigate how many FdeData are instantiated for common exception-heavy large programs? If it is not that many (so that we don't have to optimize), we could just include Fde->Sec to all FdeData.

[ruiu]

I read the code again, and I believe the easiest way to fix this without hurting the readability of the code is to move more code from EhFrameHeader::writeTo to EhFrame::getFdeData, so that getFdeData returns a ready-made .eh_frame_hdr table entries, instead of something that needs to be sorted and then be subtracted by some value. You can do that in getFdeData, not after getFdeData.

[MaskRay]

Move sorting logic from EhFrameHeader to EhFrame

[ruiu]

ELF/SyntheticSections.h

83–84

Now you can use uint32_t, no?

[MaskRay]

ELF/SyntheticSections.cpp

513

uint32_t Pc -> uint64_t PcRel
uint32_t FdeVA -> uint64_t FdeVARel

[MaskRay]

Use uint32_t for PcRel and FdeVARel

[ruiu]

ELF/SyntheticSections.cpp

515

I think you can use isInt<32>(Pc - VA).

521–527

Why did you inline only one and not the two? Maybe you should keep the original code.

[MaskRay]

Use isInt<32> and Less

[ruiu]

LGTM

Thanks!

ELF/SyntheticSections.h

public:
   size_t getSize() const override { return Size; }
 
   template <class ELFT> void addSection(InputSectionBase *S);
 
   std::vector<EhInputSection *> Sections;
   size_t NumFdes = 0;
 
   struct FdeData {
-    uint32_t Pc;
+    uint64_t Pc;
     uint32_t FdeVA;
   };
 
   std::vector<FdeData> getFdeData() const;
   ArrayRef<CieRecord *> getCieRecords() const { return CieRecords; }
 
 private:
   // This is used only when parsing EhInputSection. We keep it here to avoid
   // allocating one for each EhInputSection.
   llvm::DenseMap<size_t, CieRecord *> OffsetToCie;

ELF/SyntheticSections.cpp

 
 // Returns data for .eh_frame_hdr. .eh_frame_hdr is a binary search table
 // to get an FDE from an address to which FDE is applied. This function
 // returns a list of such pairs.
 std::vector<EhFrameSection::FdeData> EhFrameSection::getFdeData() const {
   uint8_t *Buf = getParent()->Loc + OutSecOff;
   std::vector<FdeData> Ret;
 
+  uint64_t VA = InX::EhFrameHdr->getVA();
   for (CieRecord *Rec : CieRecords) {
     uint8_t Enc = getFdeEncoding(Rec->Cie);
     for (EhSectionPiece *Fde : Rec->Fdes) {
       uint64_t Pc = getFdePc(Buf, Fde->OutputOff, Enc);
-      if (Pc > UINT32_MAX)
-        fatal(toString(Fde->Sec) + ": PC address is too large: " + Twine(Pc));
+      if (Pc - VA != (uint64_t)signExtend(Pc - VA, 32))
+        fatal(toString(Fde->Sec) + ": PC offset is too large: 0x" +
+              Twine::utohexstr(Pc - VA));
       uint32_t FdeVA = getParent()->Addr + Fde->OutputOff;
-      Ret.push_back({(uint32_t)Pc, FdeVA});
+      Ret.push_back({Pc, FdeVA});
     }
   }
   return Ret;
 }
 
 static uint64_t readFdeAddr(uint8_t *Buf, int Size) {
   switch (Size) {
   case DW_EH_PE_udata2:
     return read16(Buf);
   case DW_EH_PE_udata4:
     return read32(Buf);
   case DW_EH_PE_udata8:
     return read64(Buf);
   case DW_EH_PE_absptr:
     return readUint(Buf);
   }
   fatal("unknown FDE size encoding");
 }
 
 // Returns the VA to which a given FDE (on a mmap'ed buffer) is applied to.
 // We need it to create .eh_frame_hdr section.

test/ELF/Inputs/eh-frame-pcrel-overflow.s

+.text
+.global foo
+foo:
+  ret
+
+.section .eh_frame, "a"
+  .long 12   # Size
+  .long 0x00 # ID
+  .byte 0x01 # Version.
+
+  .byte 0x52 # Augmentation string: 'R','\0'
+  .byte 0x00
+
+  .byte 0x01
+
+  .byte 0x01 # LEB128
+  .byte 0x01 # LEB128
+
+  .byte 0x00 # DW_EH_PE_absptr
+
+  .byte 0xFF
+
+  .long 12  # Size
+  .long 0x14 # ID
+  .quad foo + 0x90000000

test/ELF/eh-frame-pcaddr-overflow.s

-# REQUIRES: x86
-
-# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
-# RUN: not ld.lld --eh-frame-hdr --section-start .text=0x1000000000000000 \
-# RUN:   %t.o -o /dev/null 2>&1 | FileCheck %s
-# CHECK: error: {{.*}}.o:(.eh_frame): PC address is too large: 2387527121043355528
-
-.text
-.global foo
-foo:
- nop
-
-.section .eh_frame, "a"
-  .long 12   # Size
-  .long 0x00 # ID
-  .byte 0x01 # Version.
-  
-  .byte 0x52 # Augmentation string: 'R','\0'
-  .byte 0x00
-  
-  .byte 0x01
-  
-  .byte 0x01 # LEB128
-  .byte 0x01 # LEB128
-
-  .byte 0x00 # DW_EH_PE_absptr
-
-  .byte 0xFF
- 
-  .long 12  # Size
-  .long 0x14 # ID
-  .quad foo + 0x1122334455667788

test/ELF/eh-frame-pcrel-overflow.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+# RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/eh-frame-pcrel-overflow.s -o %t1.o
+# RUN: ld.lld --eh-frame-hdr -Ttext=0x90000000 %t.o -o /dev/null
+# RUN: not ld.lld --eh-frame-hdr %t.o %t1.o -o /dev/null 2>&1 | FileCheck %s
+# CHECK: error: {{.*}}.o:(.eh_frame): PC offset is too large: 0x90000eac
+
+.text
+.global _start
+_start:
+  ret
+
+.section .eh_frame, "a"
+  .long 12   # Size
+  .long 0x00 # ID
+  .byte 0x01 # Version.
+
+  .byte 0x52 # Augmentation string: 'R','\0'
+  .byte 0x00
+
+  .byte 0x01
+
+  .byte 0x01 # LEB128
+  .byte 0x01 # LEB128
+
+  .byte 0x00 # DW_EH_PE_absptr
+
+  .byte 0xFF
+
+  .long 12  # Size
+  .long 0x14 # ID
+  .quad _start + 0x70000000