[PATCH] D55629: [elfabi] Add support for reading DT_SONAME from binaries

Mon Jan 7 03:47:02 PST 2019

jhenderson added inline comments.

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-arch.test:16-19
+    Content:         "0a0000000000000001000000000000000500000000000000b80200000000000000000000000000000000000000000000"
+      # DT_STRSZ      1 (0x1)
+      # DT_STRTAB     0x02b8
+      # DT_NULL       0x0
----------------
I would whole-heartedly support extending yaml2obj to allow for dynamic tags to be specified in a more readable format, possibly similar to how relocations or symbols are written?

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-arch.test:21
+    Size:            48
+  - Name:            .dynsym
+    Type:            SHT_DYNSYM
----------------
Do you actually need a .dynsym for this and the other tests?

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-arch.test:27
+    EntSize:         24
+    Content:         "000000000000000000000000000000000000000000000000"
+    Link:            .dynstr
----------------
Can you just replace this with an explicit size?

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-arch.test:33
+    Address:         0x02b8
+#                     \0
+    Content:         "00"
----------------
I'm not sure I understand the purpose of this comment... I think it's fairly obvious what the content is!

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-bad-soname.test:46
+
+# CHECK: DT_SONAME string offset outside of dynamic string table
----------------
Perhaps worth specifying the string offset in this error message?

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-bad-vaddr.test:1
+# RUN: yaml2obj %s > %t
+# RUN: not llvm-elfabi --elf %t --emit-tbe=%t.tbe 2>&1 | FileCheck %s
----------------
Which address is the "bad vaddr"? I assume it's the DT_STRTAB, in which case, I'd prefer the test to be called "binary-read-bad-dt-strtab" or similar.

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-bad-vaddr.test:16
+    Content:         "0e000000000000000d000000000000000a000000000000001c000000000000000500000000000000600200000000000000000000000000000000000000000000"
+      # DT_SONAME     13 (0x0d)
+      # DT_STRSZ      28 (0x1c)
----------------
You could actually just have DT_SONAME 0, and a single null byte in the .dynstr to simplify this test slightly. Same goes for other error cases.

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-bad-vaddr.test:38
+    PAddr: 0x1000
+    Sections:
+      - Section: .dynstr
----------------
Should this list include .dynamic too?

In general, I'm noticing some inconsistencies in the tests in what is in which segment. Could you try to unify them as much as possible, assuming that the differences aren't important?

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-bad-vaddr.test:49
+# CHECK: No PT_LOAD segment contained vaddr 0x0000000000000260 when converting .dynstr address to an offset
+
----------------
Nit: unnecessary blank line here.

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-soname-no-null.test:34-35
+    Address:         0x1018
+#                     \0 b a z\0 n o t\0 b a r\0 s o m e l i b . s o z z z\0
+    Content:         "0062617a006e6f740062617200736f6d656c69622e736f7a7a7a00"
+ProgramHeaders:
----------------
This is confusing. There's a '\0' on the end of the contents according to the section. Perhaps it should be removed?

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-soname.test:34
+    Address:         0x1018
+#                     \0 b a z\0 n o t\0 b a r\0 s o m e l i b . s o\0 f o o\0
+    Content:         "0062617a006e6f740062617200736f6d656c69622e736f00666f6f00"
----------------
To simplify things slightly, remove the "not\0bar\0" content. We only really need one thing before and one thing after the soname string.

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-soname.test:52
+# CHECK-NEXT: TbeVersion: {{[1-9]\d*\.(0|([1-9]\d*))}}
+# CHECK-NEXT: SoName: somelib.so
+# CHECK-NEXT: Arch: x86_64
----------------
Add a {{$}} at the end of this string, to make sure it doesn't have anything after it (this would match somelib.sofoo for example).

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-zero-dt-strsz.test:1
+# RUN: yaml2obj %s > %t
+# RUN: not llvm-elfabi --elf %t --emit-tbe=%t.tbe 2>&1 | FileCheck %s
----------------
I'm not sure we need this case, unless you want to explicitly check the validity of the .dynstr with llvm-elfabi. If it is zero-sized, the check for whether DT_SONAME is in it will achieve this.

================
Comment at: llvm/test/tools/llvm-elfabi/binary-read-zero-dt-strtab.test:47
+
+# CHECK: DT_STRTAB offset cannot be zero
----------------
Why not? There's nothing requiring the first loadable segment to contain the elf header and program header table. A zero address is therefore possible for .dynstr.

Also, all these errors should be "address" not "offset", since that's what they refer to.

================
Comment at: llvm/test/tools/llvm-elfabi/replace-soname-tbe.test:1
 # RUN: yaml2obj %s > %t
+# RUN: llvm-elfabi --elf %t --emit-tbe=- --soname=best.so | FileCheck %s
----------------
This would be better named "insert-soname". A separate test for what to do if there is already a SONAME is also needed.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:42
+/// @param Offset The start index of the desired substring.
+static Expected<StringRef> terminatedSubstr(StringRef Str, size_t Offset) {
+  size_t StrEnd = Str.find('\0', Offset);
----------------
This and the following function don't need to be in the anonymous namespace. I also don't think the `DynamicEntries` struct needs to be either.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:74-75

+/// This function populates a DynamicEntries struct using a binary
+/// ELFT::DynRange. After populating the struct, the members are validated with
+/// some basic sanity checks.
----------------
I'm not sure I understand what "binary" is referring to here.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:79
+/// @param Dyn Target DynamicEntries struct to populate.
+/// @param DynTable Source binary dynamic table.
+template <class ELFT>
----------------
I don't think you need to specify "binary" here. Theoretically, this function would work just as well if the table was created in memory rather than read from a file.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:95
+      if (Dyn.StrTabAddr == 0)
+        return createError("DT_STRTAB offset cannot be zero");
+      break;
----------------
As noted in the test, this isn't true.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:100
+      if (Dyn.StrSize == 0)
+        return createError("DT_STRSZ must be greater than zero");
+      break;
----------------
As noted in the test, this probably isn't a necessary check.

================
Comment at: llvm/tools/llvm-elfabi/ELFObjHandler.cpp:130
+static Expected<uint64_t> addrAsOffset(typename ELFT::PhdrRange PHdrs,
+                                       uint64_t Addr, uint64_t Range = 1) {
+  for (auto &Entry : PHdrs) {
----------------
jakehehrlich wrote:
> This hole Range thing is super confusing to me. Just have Addr and Size and just demand the user supplies Size.
I feel like this has been done before in LLVM. At least llvm-objcopy (?) has something very similar. Do we need to reinvent the wheel?

@jakehehrlich - can you remember exactly where this was?

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55629