[yaml2obj] Don't use uninitialized Type

[vitalybuka]

Alternative to #123137

With -DMACHINE=EM_NONE, machine specific
sections, like SHT_ARM_EXIDX, will fall to parse
and set Type.

This triggers msan on

yaml2obj llvm-project/llvm/test/tools/yaml2obj/ELF/mips-abi-flags.yaml -DMACHINE=EM_NONE

[llvmbot]

@llvm/pr-subscribers-llvm-support

@llvm/pr-subscribers-objectyaml

Author: Vitaly Buka (vitalybuka)

Changes

With -DMACHINE=EM_NONE, machine specific
sections, like SHT_ARM_EXIDX, will fall to parse
and set Type.

---

Full diff: https://github.com/llvm/llvm-project/pull/123274.diff

1 Files Affected:

• (modified) llvm/lib/ObjectYAML/ELFYAML.cpp (+6-3)

diff --git a/llvm/lib/ObjectYAML/ELFYAML.cpp b/llvm/lib/ObjectYAML/ELFYAML.cpp
index 83e6cf76dd746f..961815392a1348 100644
--- a/llvm/lib/ObjectYAML/ELFYAML.cpp
+++ b/llvm/lib/ObjectYAML/ELFYAML.cpp
@@ -1588,7 +1588,7 @@ static bool isInteger(StringRef Val) {
 
 void MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::mapping(
     IO &IO, std::unique_ptr<ELFYAML::Chunk> &Section) {
-  ELFYAML::ELF_SHT Type = ELF::ET_NONE;
+  ELFYAML::ELF_SHT Type;
   StringRef TypeStr;
   if (IO.outputting()) {
     if (auto *S = dyn_cast<ELFYAML::Section>(Section.get()))
@@ -1599,9 +1599,12 @@ void MappingTraits<std::unique_ptr<ELFYAML::Chunk>>::mapping(
     // When the Type string does not have a "SHT_" prefix, we know it is not a
     // description of a regular ELF output section.
     TypeStr = getStringValue(IO, "Type");
-    if (TypeStr.starts_with("SHT_") || isInteger(TypeStr))
+    if (TypeStr.starts_with("SHT_") || isInteger(TypeStr)) {
       IO.mapRequired("Type", Type);
-  }
+      if (static_cast<Input&>(IO).error())
+        Type = ELF::SHT_NULL;
+    }
+   }
 
   if (TypeStr == "Fill") {
     assert(!IO.outputting()); // We don't dump fills currently.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[vitalybuka]

@jh7370 I don't this is significantly different #123137
I don't see the bug you described, TypeStr is not empty.
Missing Type: reported by parser earlier, it's not the problem.

And we can't bailout on error(), as without default: it crashes on other assertions.
For reference, this is existing llvm-project/llvm/test/tools/yaml2obj/ELF/mips-abi-flags.yaml

[MaskRay]

Zero initialize the member variable instead?

[vitalybuka]

Zero initialize the member variable instead?

Which one? EC? There is no Output::EC, only Input::EC.

[jh7370]

I think we may have been looking at different cases, though I'm not 100% certain. When I tried this out yesterday, I omitted the Type field from the YAML entirely. As a result, the getStringValue call on line 1601 resulted in an error (because Type was missing) and TypeStr was left as an empty string. Subsequently, Type was then read without being initialised.

Looking at the code and your description, in the case you've been looking at, I'm guessing you might have a Type field in the YAML that starts with SHT_ but which doesn't have a valid mapping (based on the test you described, because the machine-specific section type is used with a non-applicable machine). Is that correct?

Your fix only fixes the latter case, which is insufficient. The correct fix takes place outside the if statement at line 1602, in one form or another. If, as you described, it's not possible to simply do return from this function after seeing the invalid/missing Type value in the YAML, then we should adopt MaskRay's suggestion of zero-initialising, but of the Type variable, like you did originally, but with an SHT_NULL value like you are doing already (but at the declaration point, not here). This might be preferable to checking the error state anyway.

What do you think?

[vitalybuka]

I think we may have been looking at different cases, though I'm not 100% certain. When I tried this out yesterday, I omitted the Type field from the YAML entirely. As a result, the getStringValue call on line 1601 resulted in an error (because Type was missing) and TypeStr was left as an empty string. Subsequently, Type was then read without being initialised.

Looking at the code and your description, in the case you've been looking at, I'm guessing you might have a Type field in the YAML that starts with SHT_ but which doesn't have a valid mapping (based on the test you described, because the machine-specific section type is used with a non-applicable machine). Is that correct?

Close, there are SHT_s which are ON/OF deppending on machine type set.
But the same happens with non-existent SHT_s as well.

Your fix only fixes the latter case, which is insufficient. The correct fix takes place outside the if statement at line 1602, in one form or another. If, as you described, it's not possible to simply do return from this function after seeing the invalid/missing Type value in the YAML, then we should adopt MaskRay's suggestion of zero-initialising the field in the first place, with an SHT_NULL value like you are doing already (but at the declaration point, not here). state anyway.

To clarify "zero-initialising the field", are we talking about local variable 'Type', not a field

This might be preferable to checking the error.

EC contains some error, not sure what we can do here?

What do you think?

Yes, that essentially what was #123137 , I just copied wrong 0 constant ET_NONE there.

[jh7370]

I think we may have been looking at different cases, though I'm not 100% certain. When I tried this out yesterday, I omitted the Type field from the YAML entirely. As a result, the getStringValue call on line 1601 resulted in an error (because Type was missing) and TypeStr was left as an empty string. Subsequently, Type was then read without being initialised.
Looking at the code and your description, in the case you've been looking at, I'm guessing you might have a Type field in the YAML that starts with SHT_ but which doesn't have a valid mapping (based on the test you described, because the machine-specific section type is used with a non-applicable machine). Is that correct?

Close, there are SHT_s which are ON/OF deppending on machine type set. But the same happens with non-existent SHT_s as well.

Your fix only fixes the latter case, which is insufficient. The correct fix takes place outside the if statement at line 1602, in one form or another. If, as you described, it's not possible to simply do return from this function after seeing the invalid/missing Type value in the YAML, then we should adopt MaskRay's suggestion of zero-initialising the field in the first place, with an SHT_NULL value like you are doing already (but at the declaration point, not here). state anyway.

To clarify "zero-initialising the field", are we talking about local variable 'Type', not a field

Yes - see my edit to the original comment (looks like you were looking at the original one, which I fixed as I realised it was unclear).

This might be preferable to checking the error.

EC contains some error, not sure what we can do here?

I'm not following what this comment is saying? We can ignore the error, like we were before, if we have an initialized Type value.

See also my comment in #123280 - perhaps the fix is to do something with mapRequired itself, though I'm not sure that would necessarily be sufficient in this case.

[vitalybuka]

See also my comment in #123280 - perhaps the fix is to do something with mapRequired itself, though I'm not sure that would necessarily be sufficient in this case.

Yes, it would not be enough here.

[jh7370]

Couple of thoughts about the test, in addition to the inline comments:

1. As we identified when both looking at this, there are two separate cases we may want to consider, namely the case the current test covers (a "Type" field which doesn't start with SHT_ but also not a recognised other type) and the case you originally identified (a SHT_ type that isn't a known value, e.g. SHT_FOO). We should probably test both of these.
2. I don't think you added the case to the right test file. The specific file you added it to was for testing the SectionHeaderTable chunk. It would probably make more sense to add the cases to the section-type.yaml file.

[vitalybuka]

Couple of thoughts about the test, in addition to the inline comments:

1. As we identified when both looking at this, there are two separate cases we may want to consider, namely the case the current test covers (a "Type" field which doesn't start with SHT_ but also not a recognised other type) and the case you originally identified (a SHT_ type that isn't a known value, e.g. SHT_FOO). We should probably test both of these.
2. I don't think you added the case to the right test file. The specific file you added it to was for testing the SectionHeaderTable chunk. It would probably make more sense to add the cases to the section-type.yaml file.

Done both.

[jh7370]

LGTM, once remaining nits fixed.