[llvm] [llvm-objdump][ELF]Fix crash when reading strings from .dynstr (PR #125679)

[James Henderson via llvm-commits]

================
@@ -221,6 +221,28 @@ template <class ELFT> void ELFDumper<ELFT>::printDynamicSection() {
   std::string TagFmt = "  %-" + std::to_string(MaxLen) + "s ";
 
   outs() << "\nDynamic Section:\n";
+  typename ELFT::Xword StringTableSize{0};
+  for (const typename ELFT::Shdr &Sec : cantFail(Elf.sections())) {
+    if (Sec.sh_type == ELF::SHT_DYNSYM) {
----------------
jh7370 wrote:

> That’s true, but `private-headers.test` will fail if `SHT_DYNAMIC` is used since there’s no `Link` to `.dynstr` in this test

That's a bug in the test (and arguably yaml2obj, since really yaml2obj should auto-link to a generated .dynstr, in the ideal world - if you fancy getting stuck into that, I'm not going to object).

That being said, your comment raises an interesting point, and I keep coming back to this: why aren't we simply using the `getDynamicStrTab` function's returned `StringRef::size` to make sure we aren't reading past the end of the data? Looking at the code, the problem is that in its current form, you can't: when `getDynamicStrTab`  finds the string table from the dynamic tags (i.e. find `DT_STRTAB`), it looks up the data using the specified address, but then simply returns:
```
      return StringRef(reinterpret_cast<const char *>(*MappedAddrOrError));
```
This overload of the `StringRef` constructor is similar to the equivalent `std::string` constructor which just looks for the first null byte and terminates the string there. However, for an ELF spec-conforming string table, this will be the very first byte in the table, since the table is lead with a null terminator (and even if it didn't have the leading terminator, every string in the table will have one, so only the first string will be included within the size). The code should instead use `DT_STRSZ` to identify the dynamic string table size in this context. If it's missing, it should probably just refuse to use the string table specified by the dynamic tags (printing a warning as it does so, since `DT_STRSZ` is listed as mandatory in the ELF spec). Arguably, this would be a regression in behaviour (previously, it was possible to use llvm-objdump to get the strings and now it wouldn't be, if the DT_STRSZ is missing and there are no sections), but as the file doesn't conform to the ELF spec, I don't think that's an issue per se. We could look at using the end of the object file as a value for the size in this case, as an alternative.

Once we have the size, you can use the `StringRef` constructor in `getDynamicStrTab` that takes the size of data as a parameter. Then, when dumping the dynamic table, you can check the tag values against the size of the returned data, to make sure you're not reading past the end.

Regarding why `getDynamicStrTab` is using `SHT_DYNSYM`, I don't know. I actually reviewed the original implementation of this (7 years ago - I didn't realise I'd been involved with LLVM for so long!), but must have missed that it was using `SHT_DYNSYM`. I did comment on something possibly related, but I suspect over the course of the review things changed in a way that I didn't notice it. Either way, since `getDynamicStrTab` is only used by the dynamic entry dumping code, I think it would be reasonable to change that to finding it via `SHT_DYNAIMC` only. If tests fail as a result of that change, let me know and we can take a look at them to see if there's some case I've missed or if the test just needs updating.

And yes, yaml2obj does (sometimes) add a SHT_DYNSYM to an ELF. I believe `DynamicSymbols` has to be specified in the ELF, but I haven't checked that one.

https://github.com/llvm/llvm-project/pull/125679