[PATCH] D43819: [ELF] - Restrict section offsets that exceeds file size.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: ELF/Writer.cpp:1904-1905
+  // It is possible to get file offset overlaps or overflows with linker
+  // script. We perform checks required in checkNoOverlappingSections() and want
+  // to fully restrict file size overflows here because it would crash linker.
+  for (OutputSection *Sec : OutputSections) {
----------------
"with linker script" -> "with linker scripts"
"want to fully restrict" -> "want to prevent"
"would crash linker" -> "would crash the linker"


================
Comment at: ELF/Writer.cpp:1907
+  for (OutputSection *Sec : OutputSections) {
+    bool Overflow = Sec->Offset >= FileSize;
+    if (Sec->Type != SHT_NOBITS)
----------------
I don't have a strong feeling here either way, but I wonder if we should worry about empty sections at all? Maybe they shouldn't cause the file to be resized? Also, why do we care whether the start of a NOBITS section is in the file?

Also, assuming the intended behaviour is that the section can start at the end of the file, if it is empty or NOBITS, I think the Overflow check is slightly wrong. If a NOBITS or empty section starts at the very end of the file (i.e. Sec->Offset == FileSize), it will be treated as overflowing as things stand.


================
Comment at: ELF/Writer.cpp:1913
+            rangeToString(Sec->Offset, Sec->Offset + Sec->Size) +
+            ", check your linker script for overflows");
+  }
----------------
", check" -> ". Check"


================
Comment at: test/ELF/linkerscript/sections-va-overflow.s:6
+# RUN: echo "SECTIONS { . = 0xffffffff20000000;" >> %t.script
+# RUN: echo "  .text : { *(.text*) } : ph_text .test 0x1000 : { BYTE(0) } }" >> %t.script
+# RUN: not ld.lld -o %t.so --script %t.script %t.o 2>&1 | FileCheck %s -check-prefix=ERR
----------------
Could you split this line into two, please, one per section.


================
Comment at: test/ELF/linkerscript/sections-va-overflow.s:11
+## with VA 0xffffffff20000000. That might be technically correct, but most probably
+## is a result of broken script file and causes file offset calculation overlow.
+## Seems we do not have to support it, so we don't and we report error in this case.
----------------
result of broken -> result of a broken
overlow -> overflow


================
Comment at: test/ELF/linkerscript/sections-va-overflow.s:12
+## is a result of broken script file and causes file offset calculation overlow.
+## Seems we do not have to support it, so we don't and we report error in this case.
+# ERR: error: unable to place section .text at file offset [0xFFFFFFFF20000000 -> 0xFFFFFFFE40000000], check your linker script for overflows
----------------
Seems -> It seems
we report error -> we report an error


https://reviews.llvm.org/D43819