[Object][COFF] Fix section name parsing error when the name field is not null-padded

[pzheng]

Summary

Some object files produced by Mirosoft tools contain sections whose name field
is not fully null-padded at the end. Microsoft's dumpbin is able to print the
section name correctly, but this causes parsing errors with LLVM tools.

So far, this issue only seems to happen when the section name is longer than 8
bytes. In this case, the section name field contains a slash (/) followed by the
offset into the string table, but the name field is not fully null-padded at the
end.

[mstorsjo]

The change itself LGTM

llvm/test/tools/llvm-objdump/COFF/long-section-name.test

20

As the replacement is hardcoded to contain /4 here, I think it feels slightly inconsistent to take the pattern /4 as a parameter. This test wouldn't really work with any other parameter than /4 (even if the symbol would constain a different offset than 4, then the replacement pattern would need to have the same offset too.

27

I was wondering about whether the test is slightly brittle - if the pattern isn't found at all, I presume that this process would fail somehow (if pos returns as -1). I'd just want to avoid that the test silently keeps passing if the offset turns out different for some reason.

Maybe it'd be safest with a check like if pos == -1: exit(1) to make it clearer?

[pzheng]

Update the test to address @mstorsjo's comments.

[pzheng]

llvm/test/tools/llvm-objdump/COFF/long-section-name.test

20

I agree, it's better to just hard code /4 into the test.

27

Makes sense to me too. Thanks for the suggestion!

[mstorsjo]

LGTM, thanks!

llvm/test/tools/llvm-objdump/COFF/long-section-name.test

19

Nitpick (and/or I'm not so well versed in python), couldn't this be just b'/4' like the line below it?

[pzheng]

Small update the test

[pzheng]

llvm/test/tools/llvm-objdump/COFF/long-section-name.test

19

Good catch! Just updated the test to only use b''.

[rnk]

llvm/lib/Object/COFFObjectFile.cpp

1171

I think it's a bug that getAsInteger doesn't work on non-null terminated StringRefs. It's not an invariant that StringRefs are null terminated. We explicitly form a non-null terminated StringRef on line 1161 above.

[pzheng]

llvm/lib/Object/COFFObjectFile.cpp

1171

hmm..., not sure if getAsInteger is supposed to handle a situation like this, the description of the function isn't very clear, but I tend to agree with you that this could be bug unless there are already code in LLVM which assumes getAsInteger should fail given such input.

[pzheng]

llvm/lib/Object/COFFObjectFile.cpp

1171

Looking at the implementation of getAsInteger, it looks like it's actually supposed to fail with such input where only the first part of it is a valid integer. getAsInteger requires the whole string to be consumed or else it's considered as a failure.

[efriedma]

llvm/lib/Object/COFFObjectFile.cpp

1171

In the testcase, Name.substr(1) contains the value "4\0abcde", i.e. an embedded null. Since '\0' isn't a digit, getAsInteger() is correctly rejecting it. consumeInteger() just stops parsing at '\0'.

I suspect this code shouldn't be passing down a StringRef with embedded nulls, though. Maybe the if (Sec->Name[COFF::NameSize - 1] == 0) check is wrong. (The spec says "null-padded", but maybe in practice Microsoft tools just treat it as "null-terminated".)

[pzheng]

llvm/lib/Object/COFFObjectFile.cpp

1171

Interesting point. The object which originally exposed the issue has an embedded null right after "/4" and some other garbage characters after that embedded null. Therefore, I used that in my test case. However, dumpbin is able to parse the section name correctly even if the string does not have an embedded null, e.g., "/4abcdef". So, modifying the if (Sec->Name[COFF::NameSize - 1] == 0) check to not pass down a StringRef with embeded nulls alone is probably not enough to prevent getAsInteger from failing with inputs like "/4abcdef", and we may still end up having to use consumeInteger to ignore the junks after "/4".

[mstorsjo]

llvm/lib/Object/COFFObjectFile.cpp

1171

FWIW, even if dumpbin accepts a string like /4abcdef I'm not sure if we strictly need to accept that - if there's no observed cases of such object files in the wild. But tolerating /4\0abcdef seems reasonable to me.

[rnk]

llvm/lib/Object/COFFObjectFile.cpp

1171

OK, I think I misunderstood the issue. I think the change is fine as is.

llvm/lib/Object/COFFObjectFile.cpp

COFFObjectFile::getSectionName(const coff_section *Sec) const {
   // Check for string table entry. First byte is '/'.
   if (Name.startswith("/")) {
     uint32_t Offset;
     if (Name.startswith("//")) {
       if (decodeBase64StringEntry(Name.substr(2), Offset))
         return createStringError(object_error::parse_failed,
                                  "invalid section name");
     } else {
-      if (Name.substr(1).getAsInteger(10, Offset))
+      if (Name.substr(1).consumeInteger(10, Offset))
         return createStringError(object_error::parse_failed,
                                  "invalid section name");
     }
     return getString(Offset);
   }
 
   return Name;
 }

llvm/test/tools/llvm-objdump/COFF/Inputs/long-section-name.yaml

+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_ARM64
+  Characteristics: [  ]
+sections:
+  - Name:            LongSectionName
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+symbols:
+  - Name:            LongSectionName
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+...

llvm/test/tools/llvm-objdump/COFF/long-section-name.test

+# RUN: yaml2obj %S/Inputs/long-section-name.yaml -o %t.obj
+
+## Replace the section name field of the object file with /4\0abcde emulating
+## a section name field not fully null-padded at the end.
+# RUN: %python %s %t.obj
+
+## This should print the LongSectionName section.
+# RUN: llvm-objdump --headers %t.obj | FileCheck %s
+
+# CHECK: LongSectionName
+
+import sys
+
+if len(sys.argv) < 2:
+  print("Use: python3 long-section-name.test <OBJECT_FILE>")
+  exit(1)
+
+pattern = b'/4'
+replacement = b'/4\0abcde'
+
+data = None
+with open(sys.argv[1], "rb") as inp:
+  data = inp.read()
+with open(sys.argv[1], "wb") as outp:
+  pos = data.find(pattern)
+  if pos == -1:
+    sys.exit("Error: Pattern /4 not found in " + sys.argv[1])
+  outp.write(data[:pos])
+  outp.write(replacement)
+  outp.write(data[pos + len(replacement):])