[PATCH] D68249: [llvm-objdump] Don't throw error for empty dynamic section

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D68249#1689097 <https://reviews.llvm.org/D68249#1689097>, @MaskRay wrote:

> I would argue that this is indeed an invalid case, though objdump -p does not error.
>
> http://www.sco.com/developers/gabi/latest/ch5.dynamic.html#dynamic_section
>
> > Except for the DT_NULL element at the end of the array, and the relative order of DT_NEEDED elements, entries may appear in any order. Tag values not appearing in the table are reserved.
>
> I take it that there should be a DT_NULL element at the end, so this section cannot be empty. The `if (Dyn.back().d_tag != ELF::DT_NULL)` test below becomes slightly more complex: "dynamic section must end with a DT_NULL" to "non-empty dynamic section must end with a DT_NULL".
>
> That said, -p emitting an error may be a bit inconvenient because -x otherwise does not emit any error.
>
> Maybe we should test: !empty or SHT_NOBITS?
>
> BTW, do you have an opinion on D67090 <https://reviews.llvm.org/D67090>?


I agree that an empty dynamic table is strictly illegal, but also that it's not ideal that we emit an error here for this specific case. However, I'm not sure testing NOBITS will work, because in this case, the range is taken from the PT_DYNAMIC program header, if I'm not mistaken. In fact, my reading of the code is that the following happens when a SHT_NOBITS .dynamic section is present.

Assumed file layout:

PT_DYNAMIC p_filesz = 0
.dynamic SHT_NOBITS

1. Search for PT_DYNAMIC segment. Find it.
2. Load data from the segment. Resultant `Dyn` is empty (because p_filesz is 0).
3. `Dyn` is empty, so attempt to load data from section.
4. No SHT_DYNAMIC section is found, so `Dyn` remains empty.
5. `Dyn` is empty, so emit an error.

As an aside, the `dynamicEntries` code will emit an error if there is no dynamic data found at all (i.e. there is no PT_DYNMIC or SHT_DYNAMIC section). I'd argue that this is incorrect (if neither is found, an empty range should be returned). However, that's somewhat irrelevant to the case at hand, since the PT_DYNAMIC is still present.



================
Comment at: llvm/lib/Object/ELF.cpp:541
 
-  if (Dyn.empty())
-    // TODO: this error is untested.
-    return createError("invalid empty dynamic section");
-
-  if (DynSecSize % sizeof(Elf_Dyn) != 0)
-    // TODO: this error is untested.
-    return createError("malformed dynamic section");
-
-  if (Dyn.back().d_tag != ELF::DT_NULL)
-    // TODO: this error is untested.
-    return createError("dynamic sections must be DT_NULL terminated");
+  if (!Dyn.empty()) {
+    if (DynSecSize % sizeof(Elf_Dyn) != 0)
----------------
grimar wrote:
> DT_NULL is a mandatory field according to the spec. I think it is invalid to remove `if (Dyn.empty())` check here.
> lib/Object is a library that is used not only by `llvm-objdump`, but by many our tools.
> For example, `dynamicEntries()` is also used by `llvm-elfabi` tool (though I am not familar with it, but anyways,
> I guess it does not want to accept an invalid dynamic section).
> 
> Perhaps, you should add a flag like `bool AllowEmpty` or do a change on a `llvm-objdump` side instead.
As empty dynamic sections are invalid, perhaps the best thing to do is to capture the errors within llvm-objdump code and report them as warnings instead. Not sure about that though, as it would mean that a user would always get a warning if they wanted to do -x on the output of --only-keep-debug.


================
Comment at: llvm/test/tools/llvm-objdump/elf-keep-debug-only.test:1
+# 'objcopy --only-keep-debug' will create objects with empty dynamic section,
+# llvm-objcopy should not crash for these files.
----------------
MaskRay wrote:
> We can move this test into elf-dynamic-section.test or rename it to elf-dynamic-section-*.test.
I'd go with `elf-dyanmic-section-empty.test`.

A lot of the newer tests in the LLVM binutils use '##' to mark comments, to distinguish them from lit `RUN:` and FileCheck `CHECK:` lines. It would be good if you do that here too.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68249