[ELF] - Don't segfault when accessing location counter inside MEMORY command.

[grimar]

Summary

We would previously crash on next script:
MEMORY { name : ORIGIN = .; }

Patch fixes that.

[ruiu]

CurAddressState is used at various places in this file, and it doesn't seem like this is the only place that you can trigger a null pointer access. Are you sure that this fix is enough?

[grimar]

In D36138#827316, @ruiu wrote:

CurAddressState is used at various places in this file, and it doesn't seem like this is the only place that you can trigger a null pointer access. Are you sure that this fix is enough?

I am trying to fix issues I know about + look around for similar cases. This issue happened because CurAddressState was accessed before processing sections commands,
where it is assigned initially. At this moment I do not know about other similar cases which can trigger null pointer access to CurAddressState.
Though it is probably can be possible, do you know any ?

I think we can go step by step probably and see if we need some generic helper or something if see another cases with CurAddressState.

[peter.smith]

I don't know whether it helps much at all, but here goes. The idea behind CurAddressState was to make sure we kept together all the information that needed to be cleared between successive calls to assignAddresses(). The two places I could spot from our existing test cases was that processCommands() and assignAddresses() that traversed the command list and evaluated expressions so that is where I thought that getSymbolValue() would actually be evaluated.

I think that this one must come from readMemoryAssignment( ) in ScriptParser as it calls readExpr()().getValue() and if readExpr happens to return "." then we will evaluate "." which to my understanding isn't allowed as the manual says "The expression must evaluate to a constant before memory allocation is performed, which means that you may not use any section relative symbols."

The places that could go wrong and access a Null Pointer are I think restricted to where .getValue() is called on an expression containing "." outside the context of processCommands() or assignAddresses(), which I believe is where the ScriptParser is expecting a constant. As such I think the fix should at least give an error message, and if you are looking for more test cases then using "." in areas where the parser is expecting a constant will be a good place to look.

For reference CurAddressState was put in under D34345, although comments will probably need to be found on the list as I don't think that they automatically get added to Phab..

[grimar]

ping

[ruiu]

LGTM

test/ELF/linkerscript/memory-err.s

1 ↗

(On Diff #109079)

Rename memory-error.s (you don't need to save two characters in filename.)

[grimar]

test/ELF/linkerscript/memory-err.s

1 ↗

(On Diff #109079)

Ok, though we have many tests with both ways of naming atm.

[grimar]

test/ELF/linkerscript/memory-err.s

1 ↗

(On Diff #109079)

Ah and we already have memory-err.s one committed.
I'll leave name unchanged for commit.

Should we rename all *-err.s to *-error.s at once then ?

lld/trunk/ELF/LinkerScript.cpp

if (Opt.PhdrsCommands.empty())
     return false;
   for (PhdrsCommand &Cmd : Opt.PhdrsCommands)
     if (Cmd.Type == PT_INTERP)
       return false;
   return true;
 }
 
 ExprValue LinkerScript::getSymbolValue(const Twine &Loc, StringRef S) {
-  if (S == ".")
-    return {CurAddressState->OutSec, Dot - CurAddressState->OutSec->Addr, Loc};
+  if (S == ".") {
+    if (CurAddressState)
+      return {CurAddressState->OutSec, Dot - CurAddressState->OutSec->Addr,
+              Loc};
+    error(Loc + ": unable to get location counter value");
+    return 0;
+  }
   if (SymbolBody *B = Symtab->find(S)) {
     if (auto *D = dyn_cast<DefinedRegular>(B))
       return {D->Section, D->Value, Loc};
     if (auto *C = dyn_cast<DefinedCommon>(B))
       return {InX::Common, C->Offset, Loc};
   }
   error(Loc + ": symbol not found: " + S);
   return 0;

lld/trunk/test/ELF/linkerscript/memory-err.s

 # REQUIRES: x86
 # RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
 # RUN: echo "MEMORY { name : ORIGIN = DATA_SEGMENT_RELRO_END; }" > %t.script
 # RUN: not ld.lld -shared -o %t2 --script %t.script %t 2>&1 | FileCheck %s
 # CHECK: error: {{.*}}.script:1: unable to calculate page size
 
 # RUN: echo "MEMORY { name : ORIGIN = CONSTANT(COMMONPAGESIZE); }" > %t.script
 # RUN: not ld.lld -shared -o %t2 --script %t.script %t 2>&1 |\
 # RUN:   FileCheck %s --check-prefix=ERR2
 # ERR2: error: {{.*}}.script:1: unable to calculate page size
+
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
+# RUN: echo "MEMORY { name : ORIGIN = .; }" > %t.script
+# RUN: not ld.lld -shared -o %t2 --script %t.script %t 2>&1 |\
+# RUN:   FileCheck %s --check-prefix=ERR3
+# ERR3: error: {{.*}}.script:1: unable to get location counter value