[PATCH] D77216: [llvm-readobj] - Fix crashes and misbehaviors when reading strings from broken string tables.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test:289
+# RUN: llvm-readobj --dynamic-table %t6 2>&1 | \
+# RUN:   FileCheck %s -DFILE=%t6 --implicit-check-not=warning --check-prefixes=NOT-TERMINATED,NOT-TERMINATED-GREQ
+# RUN: llvm-readelf --dynamic-table %t6 2>&1 | \
----------------
`--implicit-check-not=warning:` is probably better to avoid weird failures with path names involving "warning" (there's bound to be somebody out there...).

Same below.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test:293
+
+# NOT-TERMINATED:      warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb4, the string table is not null-terminated
+# NOT-TERMINATED:      warning: '[[FILE]]': string table at offset 0xb0: unable to read the string at 0xb0, the string table is not null-terminated
----------------
I might be inclined to move these checks to below all the RUN lines for the different cases, as the use of NOT-TERMINATED-LESS is a little confusing when nothing up to that point in the test file had actually speciifed it.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test:384
+
+## Note: strings are dumped because the file ends with the zero byte. Code reads the data
+## in [DT_STRTAB, DT_STRTAB + DT_STRSZ] range as a as a normal null-terminated string table.
----------------
> strings are dumped because the file ends with the zero byte

What guarantees this? Should we add something to either a) validate it (to make it easier to identify the problem if it doesn't end with a zero byte) or b) force it to always be zero?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/dynamic-malformed.test:385
+## Note: strings are dumped because the file ends with the zero byte. Code reads the data
+## in [DT_STRTAB, DT_STRTAB + DT_STRSZ] range as a as a normal null-terminated string table.
+# BEFORE-THE-EOF:      {{[(]?}}NEEDED{{[)]?}}    Shared library: [test.soabc]
----------------
Delete one of the "as a".


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2576-2578
+    return WarnAndReturn(" has size (0x" +
+                             Twine::utohexstr(DynamicStringTable.size()) +
+                             ") that goes past the end of the file (0x" +
----------------
I think you can make the message slightly more concise:

"string table at offset 0x12345678 with size 0x87654321 goes past the end of the file (0x22222222)"


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2585
+        ": unable to read the string at 0x" + Twine::utohexstr(Offset + Value) +
+            ", it goes past the end of the table (0x" +
+            Twine::utohexstr(Offset + DynamicStringTable.size()) + ")",
----------------
Unrelated I know, but this should be ':' not ',' for more correct grammar.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2592
+                             Twine::utohexstr(Offset + Value) +
+                             ", the string table is not null-terminated",
+                         Offset);
----------------
I'd change ',' to ':' here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77216/new/

https://reviews.llvm.org/D77216