[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

431

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

return RecordARMScatteredRelocation(Writer, Asm, Layout, Fragment, Fixup,
                                         FixedValue);
 
   // 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!
+  if (!A) { // constant
+    // FIXME! This is Target.isAbsolute() case as we check SymB above. We check
+    // !A to ensure that null pointer isn't dereferenced and suppress static
+    // analyzer warnings.
     report_fatal_error("FIXME: relocations to absolute targets "
                        "not yet implemented");
   } else {
     // Resolve constant variables.
     if (A->isVariable()) {
       int64_t Res;
       if (A->getVariableValue()->evaluateAsAbsolute(
               Res, Layout, Writer->getSectionAddressMap())) {