[obj2yaml] - Stop triggering UB when dumping corrupted strings.

[grimar]

Summary

We have a following code to find quote type:

if (isspace(S.front()) || isspace(S.back()))

Problem is that:

"int isspace( int ch ): The behavior is undefined if the value of
ch is not representable as unsigned char and is not equal to EOF."
(https://en.cppreference.com/w/cpp/string/byte/isspace)

This patch shows how this UB can be triggered and fixes an issue.

[MaskRay]

Thanks!

test/tools/obj2yaml/invalid-section-name.yaml

31 ↗

(On Diff #225573)

Does a shorter sequence work?

[rupprecht]

test/tools/obj2yaml/invalid-section-name.yaml

3 ↗

(On Diff #225573)

nit: remove "do"

[grimar]

test/tools/obj2yaml/invalid-section-name.yaml

31 ↗

(On Diff #225573)

For this particular test "00FE00" works fine.

But in this case another sections produced (.strtab, .shstrtab) get a sh_name larger than the section name string table size though.
e.g. sh_name=5.

Then if we use "llvm-readobj -a" for the object produced, we'll have an error like
"error: '1.o': a section [index 3] has an invalid sh_name (0xf) offset which goes past the end of the section name string table"

A minimal sequence that allowes llvm-readobj -a to pass is

Content: "00FEFEFEFEFEFEFEFEFEFEFEFEFEFE00"

I wasn't sure what to use here. I had a feeling that it is a bit tricky place and is not the best candidate for optimization,
so even added s few more "FE" just in case.

[jhenderson]

llvm/test/tools/obj2yaml/invalid-section-name.yaml

2–4

It looks like @rupprecht's comment (remove "do") here wasn't addressed prior to commit. I also want to suggest a couple of other minor improvements to the comment:

"Here we replace the section name..."
"We used to assert for..."

[grimar]

llvm/test/tools/obj2yaml/invalid-section-name.yaml

2–4

It looks like @rupprecht's comment (remove "do") here wasn't addressed prior to commit.

Yes, it was forgotten for a short time, but I addressed it in the follow-up: rL375405.

I'll apply your suggestions tomorrow.

llvm/include/llvm/Support/YAMLTraits.h

 // 5.1. Character Set
 // The allowed character range explicitly excludes the C0 control block #x0-#x1F
 // (except for TAB #x9, LF #xA, and CR #xD which are allowed), DEL #x7F, the C1
 // control block #x80-#x9F (except for NEL #x85 which is allowed), the surrogate
 // block #xD800-#xDFFF, #xFFFE, and #xFFFF.
 inline QuotingType needsQuotes(StringRef S) {
   if (S.empty())
     return QuotingType::Single;
-  if (isspace(S.front()) || isspace(S.back()))
+  if (isspace(static_cast<unsigned char>(S.front())) ||
+      isspace(static_cast<unsigned char>(S.back())))
     return QuotingType::Single;
   if (isNull(S))
     return QuotingType::Single;
   if (isBool(S))
     return QuotingType::Single;
   if (isNumeric(S))
     return QuotingType::Single;
 

llvm/test/tools/obj2yaml/invalid-section-name.yaml

+## Check we do not crash/assert when dumping a broken section name.
+## Here we replace "foo" name with a sequence of characters that
+## do are not representable as unsigned char.
+## We used to have an assert for this case before. 
+
+# RUN: yaml2obj %s -o %t
+# RUN: obj2yaml %t | FileCheck %s
+
+# CHECK:      --- !ELF
+# CHECK-NEXT: FileHeader:
+# CHECK-NEXT:   Class:   ELFCLASS64
+# CHECK-NEXT:   Data:    ELFDATA2LSB
+# CHECK-NEXT:   Type:    ET_REL
+# CHECK-NEXT:   Machine: EM_X86_64
+# CHECK-NEXT: Sections:
+# CHECK-NEXT:   - Name: "{{.*}}"
+# CHECK-NEXT:     Type: SHT_PROGBITS
+# CHECK-NEXT: ...
+
+--- !ELF
+FileHeader:
+  Class:   ELFCLASS64
+  Data:    ELFDATA2LSB
+  Type:    ET_REL
+  Machine: EM_X86_64
+Sections:
+  - Name: foo
+    Type: SHT_PROGBITS
+  - Name: .shstrtab
+    Type: SHT_STRTAB
+    Content: "00FEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFEFE00"