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

[ruiu]

ELF/SyntheticSections.cpp

509–510

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.

509–510

Can't FdeVA overflow?

[MaskRay]

Add test

[MaskRay]

ELF/SyntheticSections.cpp

509–510

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

82–83

Now you can use uint32_t, no?

[MaskRay]

ELF/SyntheticSections.cpp

509–510

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

[MaskRay]

Use uint32_t for PcRel and FdeVARel

[ruiu]

ELF/SyntheticSections.cpp

511

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;
-    uint32_t FdeVA;
+    uint64_t PcRel;
+    uint64_t FdeVARel;
   };
 
   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.

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));
-      uint32_t FdeVA = getParent()->Addr + Fde->OutputOff;
-      Ret.push_back({(uint32_t)Pc, FdeVA});
+      uint64_t FdeVA = getParent()->Addr + Fde->OutputOff;
+      if (Pc - VA != (uint64_t)signExtend(Pc - VA, 32))
+        fatal(toString(Fde->Sec) + ": PC offset is too large: 0x" +
+              Twine::utohexstr(Pc - VA));
+      Ret.push_back({Pc - VA, FdeVA - VA});
     }
   }
+
+  // Sort the FDE list by their PC and uniqueify. Usually there is only
+  // one FDE for a PC (i.e. function), but if ICF merges two functions
+  // into one, there can be more than one FDEs pointing to the address.
+  std::stable_sort(
+      Ret.begin(), Ret.end(),
+      [](const FdeData &A, const FdeData &B) { return A.PcRel < B.PcRel; });
+  auto Eq = [](const FdeData &A, const FdeData &B) {
+    return A.PcRel == B.PcRel;
+  };
+  Ret.erase(std::unique(Ret.begin(), Ret.end(), Eq), Ret.end());
+
   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:

 // Each entry of the search table consists of two values,
 // the starting PC from where FDEs covers, and the FDE's address.
 // It is sorted by PC.
 void EhFrameHeader::writeTo(uint8_t *Buf) {
   typedef EhFrameSection::FdeData FdeData;
 
   std::vector<FdeData> Fdes = InX::EhFrame->getFdeData();
 
-  // Sort the FDE list by their PC and uniqueify. Usually there is only
-  // one FDE for a PC (i.e. function), but if ICF merges two functions
-  // into one, there can be more than one FDEs pointing to the address.
-  auto Less = [](const FdeData &A, const FdeData &B) { return A.Pc < B.Pc; };
-  std::stable_sort(Fdes.begin(), Fdes.end(), Less);
-  auto Eq = [](const FdeData &A, const FdeData &B) { return A.Pc == B.Pc; };
-  Fdes.erase(std::unique(Fdes.begin(), Fdes.end(), Eq), Fdes.end());
-
   Buf[0] = 1;
   Buf[1] = DW_EH_PE_pcrel | DW_EH_PE_sdata4;
   Buf[2] = DW_EH_PE_udata4;
   Buf[3] = DW_EH_PE_datarel | DW_EH_PE_sdata4;
   write32(Buf + 4, InX::EhFrame->getParent()->Addr - this->getVA() - 4);
   write32(Buf + 8, Fdes.size());
   Buf += 12;
 
-  uint64_t VA = this->getVA();
   for (FdeData &Fde : Fdes) {
-    write32(Buf, Fde.Pc - VA);
-    write32(Buf + 4, Fde.FdeVA - VA);
+    write32(Buf, Fde.PcRel);
+    write32(Buf + 4, Fde.FdeVARel);
     Buf += 8;
   }
 }
 
 size_t EhFrameHeader::getSize() const {
   // .eh_frame_hdr has a 12 bytes header followed by an array of FDEs.
   return 12 + InX::EhFrame->NumFdes * 8;
 }

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