[LLD] [COFF] Fix crashes for cfguard with undefined weak symbols

[mstorsjo]

When marking symbols as having their address taken, we can have the sitaution where we have the address taken of a weak symbol. If there's no strong definition of the symbol, the symbol ends up as an absolute symbol with the value null. In those cases, we don't have any Chunk. Skip such symbols from the cfguard tables.

This fixes #78619.

[llvmbot]

@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-coff

Author: Martin Storsjö (mstorsjo)

Changes

When marking symbols as having their address taken, we can have the sitaution where we have the address taken of a weak symbol. If there's no strong definition of the symbol, the symbol ends up as an absolute symbol with the value null. In those cases, we don't have any Chunk. Skip such symbols from the cfguard tables.

This fixes #78619.

---

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

2 Files Affected:

• (modified) lld/COFF/Writer.cpp (+2)
• (added) lld/test/COFF/cfguard-weak-undef.s (+27)

diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index 2e34a6c5cfa2c0e..9c20bbb83d86d19 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -1802,6 +1802,8 @@ void Writer::createSEHTable() {
 // symbol's offset into that Chunk.
 static void addSymbolToRVASet(SymbolRVASet &rvaSet, Defined *s) {
   Chunk *c = s->getChunk();
+  if (!c)
+    return;
   if (auto *sc = dyn_cast<SectionChunk>(c))
     c = sc->repl; // Look through ICF replacement.
   uint32_t off = s->getRVA() - (c ? c->getRVA() : 0);
diff --git a/lld/test/COFF/cfguard-weak-undef.s b/lld/test/COFF/cfguard-weak-undef.s
new file mode 100644
index 000000000000000..fd4121ac27dfcc1
--- /dev/null
+++ b/lld/test/COFF/cfguard-weak-undef.s
@@ -0,0 +1,27 @@
+# REQUIRES: x86
+# RUN: llvm-mc -triple=x86_64-windows-gnu -filetype=obj -o %t.obj %s
+# RUN: lld-link %t.obj /out:%t.exe /entry:entry /subsystem:console /guard:cf
+
+	.def	@feat.00;
+	.scl	3;
+	.type	0;
+	.endef
+	.globl	@feat.00
+.set @feat.00, 2048
+
+	.globl	entry
+entry:
+	retq
+
+	.data
+	.globl	funcs
+funcs:
+	.quad	weakfunc
+
+	.section	.gfids$y,"dr"
+	.symidx	weakfunc
+	.section	.giats$y,"dr"
+	.section	.gljmp$y,"dr"
+	.weak	weakfunc
+	.addrsig
+	.addrsig_sym weakfunc

[alvinhochun]

Do you have an example stack trace of the codepath that triggers the error? I wonder if maybeAddAddressTakenFunction is supposed to prevent this from happening in the first place. If the problematic codepath bypasses this function and calls addSymbolToRVASet directly, then maybe it shouldn't...

[mstorsjo]

Do you have an example stack trace of the codepath that triggers the error? I wonder if maybeAddAddressTakenFunction is supposed to prevent this from happening in the first place. If the problematic codepath bypasses this function and calls addSymbolToRVASet directly, then maybe it shouldn't...

Yes, the callstack doesn't pass through maybeAddAddressTakenFunction here, we have this call stack:

#10 0x0000aaaab3da976c addSymbolToRVASet(llvm::DenseSet<lld::coff::ChunkAndOffset, llvm::DenseMapInfo<lld::coff::ChunkAndOffset, void>>&, lld::coff::Defined*) /home/martin/code/llvm-project/llvm/tools/lld/COFF/Writer.cpp:1805:40
#11 0x0000aaaab3daa500 (anonymous namespace)::Writer::markSymbolsForRVATable(lld::coff::ObjFile*, llvm::ArrayRef<lld::coff::SectionChunk*>, llvm::DenseSet<lld::coff::ChunkAndOffset, llvm::DenseMapInfo<lld::coff::ChunkAndOffset, void>>&) /home/martin/code/llvm-project/llvm/tools/lld/COFF/Writer.cpp:2016:20
#12 0x0000aaaab3da9c44 (anonymous namespace)::Writer::createGuardCFTables() /home/martin/code/llvm-project/llvm/tools/lld/COFF/Writer.cpp:1903:29

The reason is that we're not running linker heuristics to add these, but we're compiled with cfguard enabled, and those tables says that the address of this symbol is taken. We just don't know at compile time whether the symbol will evaluate to a non-null value or not.

[alvinhochun]

Lgtm, thanks.