[llvm-nm] Fix heap-use-after-free while executing 'llvm-nm -n --export-symbols'

[chbessonova]

Use symbol's flags saved in NMSymbol::SymFlags inside NMSymbol::isDefined() since BasicSymbolRef::getFlags() requires the symbol's containing entity object to exist (which doesn't, causing llvm-nm to crash).

Here is the AddressSanitizer report:

==3324663==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000000200 READ of size 8 at 0x60e000000200 thread T0
    #0 0x55c6536785d8 in llvm::object::BasicSymbolRef::getFlags() const llvm-project/llvm/include/llvm/Object/SymbolicFile.h:207:24
    #1 0x55c6536785d8 in (anonymous namespace)::NMSymbol::isDefined() const llvm-project/llvm/tools/llvm-nm/llvm-nm.cpp:241:37

[MaskRay]

Is there a test covering this case?

[MaskRay]

@tstellar It seems that https://github.com/orgs/llvm/teams/pr-subscribers-llvm-binary-utilities cannot be added as a reviewer. Perhaps the team hasn't been properly set?

[chbessonova]

@MaskRay

Is there a test covering this case?

No. I'm not sure what would be a good test for this. I can add a test that just run llvm-nm -n --export-symbols to ensure it works fine, but it'd only check that this command line run w/o issues, and it wouldn't help to catch bugs where some properties of BasicSymbolRef requested when there is no valid OwningObject set.

[llvmbot]

@llvm/pr-subscribers-llvm-binary-utilities

Changes

Use symbol's flags saved in NMSymbol::SymFlags inside NMSymbol::isDefined() since BasicSymbolRef::getFlags() requires the symbol's containing entity object to exist (which doesn't, causing llvm-nm to crash).

Here is the AddressSanitizer report:

==3324663==ERROR: AddressSanitizer: heap-use-after-free on address 0x60e000000200 READ of size 8 at 0x60e000000200 thread T0
    #0 0x55c6536785d8 in llvm::object::BasicSymbolRef::getFlags() const llvm-project/llvm/include/llvm/Object/SymbolicFile.h:207:24
    #1 0x55c6536785d8 in (anonymous namespace)::NMSymbol::isDefined() const llvm-project/llvm/tools/llvm-nm/llvm-nm.cpp:241:37

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

1 Files Affected:

• (modified) llvm/tools/llvm-nm/llvm-nm.cpp (+2-4)

diff --git a/llvm/tools/llvm-nm/llvm-nm.cpp b/llvm/tools/llvm-nm/llvm-nm.cpp
index 79213b3b2c27e31..8ac7eb2a825b57e 100644
--- a/llvm/tools/llvm-nm/llvm-nm.cpp
+++ b/llvm/tools/llvm-nm/llvm-nm.cpp
@@ -237,10 +237,8 @@ struct NMSymbol {
   std::string IndirectName;
 
   bool isDefined() const {
-    if (Sym.getRawDataRefImpl().p) {
-      uint32_t Flags = cantFail(Sym.getFlags());
-      return !(Flags & SymbolRef::SF_Undefined);
-    }
+    if (Sym.getRawDataRefImpl().p)
+      return !(SymFlags & SymbolRef::SF_Undefined);
     return TypeChar != 'U';
   }
 

[MaskRay]

Since this is purely code simplification, I think this is fine.