[ARM] Fix null pointer dereferences in ARMMachObjectWriter::recordRelocation()

[SweetVishnya]

Summary

Bugs were found by Svace static analysis tool. A can be a null pointer.
It is checked in some places. However, there are still some missing
checks.

[samtebbs]

Thanks sweetvishnya, this looks worthwhile. Do you have the link for a github issue for this?

[SweetVishnya]

In D147931#4257478, @samtebbs wrote:

Thanks sweetvishnya, this looks worthwhile. Do you have the link for a github issue for this?

I didn't create any issues on GitHub. Do I need to create a new issue for this bug?

[dmgreen]

I'm not sure A can be null here. Either getSymB will be true (handled above) or isAbsolute will be true. Else A should have a value, if Im reading it correctly.

[SweetVishnya]

In D147931#4260122, @dmgreen wrote:

I'm not sure A can be null here. Either getSymB will be true (handled above) or isAbsolute will be true. Else A should have a value, if Im reading it correctly.

The bug was reported by a static analyzer because A is checked for null in line 419. But I am not really familiar with the code here.

[MaskRay]

This is a false positive case of the static analyzer.

[dmgreen]

If you wanted to add an assert or change the condition of the if (Target.isAbsolute()) to if (!A) then I wouldn't be against it, providing there was a comment to explain that meant isAbsolute. But yes, this looks like a false positive.

[SweetVishnya]

Add !A check as @dmgreen suggested

This will suppress static analyzer warnings.

[dmgreen]

Thanks. One minor suggestion but otherwise Sounds OK to me.

llvm/lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp

433

Can you add This is Target.isAbsolute() case as we check SymB above. ..

[SweetVishnya]

@dmgreen, I fixed the comment. @MaskRay, do you agree with current resolution?

[SweetVishnya]

So, can we push this patch to main branch?

llvm/lib/Target/ARM/MCTargetDesc/ARMMachObjectWriter.cpp

void ARMMachObjectWriter::recordRelocation(MachObjectWriter *Writer,
   // See <reloc.h>.
   uint32_t FixupOffset = Layout.getFragmentOffset(Fragment)+Fixup.getOffset();
   unsigned Index = 0;
   unsigned Type = 0;
   const MCSymbol *RelSymbol = nullptr;
 
   if (Target.isAbsolute()) { // constant
     // FIXME!
     report_fatal_error("FIXME: relocations to absolute targets "
                        "not yet implemented");
   } else {
     // Resolve constant variables.
-    if (A->isVariable()) {
+    if (A && A->isVariable()) {
       int64_t Res;
       if (A->getVariableValue()->evaluateAsAbsolute(
               Res, Layout, Writer->getSectionAddressMap())) {
         FixedValue = Res;
         return;
       }
     }
 
     // Check whether we need an external or internal relocation.
+    if (A) {
       if (requiresExternRelocation(Writer, Asm, *Fragment, RelocType, *A,
                                    FixedValue)) {
         RelSymbol = A;
 
         // For external relocations, make sure to offset the fixup value to
         // compensate for the addend of the symbol address, if it was
         // undefined. This occurs with weak definitions, for example.
         if (!A->isUndefined())
           FixedValue -= Layout.getSymbolOffset(*A);
       } else {
         // The index is the section ordinal (1-based).
         const MCSection &Sec = A->getSection();
         Index = Sec.getOrdinal() + 1;
         FixedValue += Writer->getSectionAddress(&Sec);
       }
+    }
     if (IsPCRel)
       FixedValue -= Writer->getSectionAddress(Fragment->getParent());
 
     // The type is determined by the fixup kind.
     Type = RelocType;
   }
 
   // struct relocation_info (8 bytes)