[COFF] Add exported functions to gfids table for /guard:cf

[rnk]

Summary

MSVC does this, and we should to.

The .gfids table is a table of RVAs, so it's impossible for a DLL to
indicate that an imported symbol is address taken. Therefore, exports
appear to be listed as address taken by the DLL that exports them.

This fixes an issue that Firefox ran into here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1485016#c12

In Firefox, the export directive came from a .def file, but we need to
do this for any kind of export.

[alex]

I don't know this code very well at all, so just one small comment that jumped out to me. Conceptually it looks like this solves the problem we were seeing. Thanks for jumping on this so quickly!

lld/COFF/Writer.cpp

1371 ↗

(On Diff #174678)

In the case where Config->Entry is a Defined, but not a DefinedCOFF, it won't get added with this patch, whereas it would have before. Can that situation happen?

[dmajor]

Thanks! I applied the patch and confirmed that it fixes the problematic Firefox DLL. (I don't know if I'm allowed to formally approve it though...)

[hans]

lgtm

[rnk]

Thanks, landing soon.

lld/COFF/Writer.cpp

1371 ↗

(On Diff #174678)

(one week later, after the holidays...) So, it looks like that situation can happen. The one I came up with was import thunks. They can be exported, entry points, and address taken, although it's a bit ridiculous to import an export, or use an imported symbol as your entry point.

However, it's a pre-existing issue in the relocation scanning code. Even though it's a behavior change for entry point symbols, I think making them both have the same bug is actually somewhat better. I filed this bug for it: https://bugs.llvm.org/show_bug.cgi?id=39799

COFF/Writer.cpp

 static void addSymbolToRVASet(SymbolRVASet &RVASet, Defined *S) {
   Chunk *C = S->getChunk();
   if (auto *SC = dyn_cast<SectionChunk>(C))
     C = SC->Repl; // Look through ICF replacement.
   uint32_t Off = S->getRVA() - (C ? C->getRVA() : 0);
   RVASet.insert({C, Off});
 }
 
+// Given a symbol, add it to the GFIDs table if it is a live, defined, function
+// symbol in an executable section.
+static void maybeAddAddressTakenFunction(SymbolRVASet &AddressTakenSyms,
+                                         Symbol *S) {
+  auto *D = dyn_cast_or_null<DefinedCOFF>(S);
+
+  // Ignore undefined symbols and references to non-functions (e.g. globals and
+  // labels).
+  if (!D ||
+      D->getCOFFSymbol().getComplexType() != COFF::IMAGE_SYM_DTYPE_FUNCTION)
+    return;
+
+  // Mark the symbol as address taken if it's in an executable section.
+  Chunk *RefChunk = D->getChunk();
+  OutputSection *OS = RefChunk ? RefChunk->getOutputSection() : nullptr;
+  if (OS && OS->Header.Characteristics & IMAGE_SCN_MEM_EXECUTE)
+    addSymbolToRVASet(AddressTakenSyms, D);
+}
+
 // Visit all relocations from all section contributions of this object file and
 // mark the relocation target as address-taken.
 static void markSymbolsWithRelocations(ObjFile *File,
                                        SymbolRVASet &UsedSymbols) {
   for (Chunk *C : File->getChunks()) {
     // We only care about live section chunks. Common chunks and other chunks
     // don't generally contain relocations.
     SectionChunk *SC = dyn_cast<SectionChunk>(C);
     if (!SC || !SC->Live)
       continue;
 
     for (const coff_relocation &Reloc : SC->Relocs) {
       if (Config->Machine == I386 && Reloc.Type == COFF::IMAGE_REL_I386_REL32)
         // Ignore relative relocations on x86. On x86_64 they can't be ignored
         // since they're also used to compute absolute addresses.
         continue;
 
       Symbol *Ref = SC->File->getSymbol(Reloc.SymbolTableIndex);
-      if (auto *D = dyn_cast_or_null<DefinedCOFF>(Ref)) {
-        if (D->getCOFFSymbol().getComplexType() != COFF::IMAGE_SYM_DTYPE_FUNCTION)
-          // Ignore relocations against non-functions (e.g. labels).
-          continue;
-
-        // Mark the symbol if it's in an executable section.
-        Chunk *RefChunk = D->getChunk();
-        OutputSection *OS = RefChunk ? RefChunk->getOutputSection() : nullptr;
-        if (OS && OS->Header.Characteristics & IMAGE_SCN_MEM_EXECUTE)
-          addSymbolToRVASet(UsedSymbols, D);
-      }
+      maybeAddAddressTakenFunction(UsedSymbols, Ref);
     }
   }
 }
 
 // Create the guard function id table. This is a table of RVAs of all
 // address-taken functions. It is sorted and uniqued, just like the safe SEH
 // table.
 void Writer::createGuardCFTables() {
if (File->hasGuardCF()) {
       markSymbolsForRVATable(File, File->getGuardLJmpChunks(), LongJmpTargets);
     } else {
       markSymbolsWithRelocations(File, AddressTakenSyms);
     }
   }
 
   // Mark the image entry as address-taken.
   if (Config->Entry)
-    addSymbolToRVASet(AddressTakenSyms, cast<Defined>(Config->Entry));
+    maybeAddAddressTakenFunction(AddressTakenSyms, Config->Entry);
+
+  // Mark exported symbols in executable sections as address-taken.
+  for (Export &E : Config->Exports)
+    maybeAddAddressTakenFunction(AddressTakenSyms, E.Sym);
 
   // Ensure sections referenced in the gfid table are 16-byte aligned.
   for (const ChunkAndOffset &C : AddressTakenSyms)
     if (C.InputChunk->Alignment < 16)
       C.InputChunk->Alignment = 16;
 
   maybeAddRVATable(std::move(AddressTakenSyms), "__guard_fids_table",
                    "__guard_fids_count");

test/COFF/gfids-export.s

+# REQUIRES: x86
+# RUN: llvm-mc -triple x86_64-pc-win32 %s -filetype=obj -o %t.obj
+# RUN: lld-link %t.obj -guard:cf -dll -out:%t.dll -noentry
+# RUN: llvm-readobj -coff-load-config %t.dll | FileCheck %s --check-prefix=CHECK
+
+# There should be a single entry in the table for the exported symbol.
+#
+# CHECK: GuardFidTable [
+# CHECK-NEXT: 0x180001000
+# CHECK-NEXT: ]
+
+        .def    func_export; .scl    2; .type   32; .endef
+        .globl func_export
+        .section       .text,"xr",one_only,func_export
+        .p2align 4
+func_export:
+        movl $1, %eax
+        .globl label_export
+label_export:
+        movl $2, %eax
+        ret
+
+        .data
+        .globl data_export
+data_export:
+        .long 42
+
+        .section .drectve,"dr"
+        .ascii " /EXPORT:func_export"
+        .ascii " /EXPORT:label_export"
+        .ascii " /EXPORT:data_export"
+
+
+# Load configuration directory entry (winnt.h _IMAGE_LOAD_CONFIG_DIRECTORY64).
+# The linker will define the __guard_* symbols.
+        .section .rdata,"dr"
+.globl _load_config_used
+_load_config_used:
+        .long 256
+        .fill 124, 1, 0
+        .quad __guard_fids_table
+        .quad __guard_fids_count
+        .long __guard_flags
+        .fill 128, 1, 0