Skip to content

[LLD][COFF] Fix importing DllMain from import libraries #146610

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 2, 2025

Conversation

aganea
Copy link
Member

@aganea aganea commented Jul 1, 2025

This is a workaround for #82050 by skipping importing the DllMain symbol from import libraries. If this situation occurs, after this PR a warning will also be displayed. The warning can be silenced with /ignore:exporteddllmain

@llvmbot
Copy link
Member

llvmbot commented Jul 1, 2025

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

@llvm/pr-subscribers-lld

Author: Alexandre Ganea (aganea)

Changes

This is a workaround for #82050 by skipping importing the DllMain symbol from import libraries. If this situation occurs, after this PR a warning will also be displayed. The warning can be silenced with /ignore:exporteddllmain


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

4 Files Affected:

  • (modified) lld/COFF/Config.h (+1)
  • (modified) lld/COFF/Driver.cpp (+2)
  • (modified) lld/COFF/InputFiles.cpp (+39-1)
  • (added) lld/test/COFF/exported-dllmain.test (+57)
diff --git a/lld/COFF/Config.h b/lld/COFF/Config.h
index 473943a239422..79b63e5b7236f 100644
--- a/lld/COFF/Config.h
+++ b/lld/COFF/Config.h
@@ -307,6 +307,7 @@ struct Configuration {
   bool warnDebugInfoUnusable = true;
   bool warnLongSectionNames = true;
   bool warnStdcallFixup = true;
+  bool warnExportedDllMain = true;
   bool incremental = true;
   bool integrityCheck = false;
   bool killAt = false;
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 8a016d44d4636..283aeed1a19cd 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -1643,6 +1643,8 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
         config->warnLocallyDefinedImported = false;
       else if (s == "longsections")
         config->warnLongSectionNames = false;
+      else if (s == "exporteddllmain")
+        config->warnExportedDllMain = false;
       // Other warning numbers are ignored.
     }
   }
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 760ce00dda0d6..0b7dbea8cdd99 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -114,6 +114,35 @@ static coff_symbol_generic *cloneSymbol(COFFSymbolRef sym) {
   }
 }
 
+// Skip importing DllMain thunks from import libraries.
+static bool fixupDllMain(COFFLinkerContext &ctx, llvm::object::Archive *file,
+                         const Archive::Symbol &sym, bool &skipDllMain) {
+  if (skipDllMain)
+    return true;
+  const Archive::Child &c =
+      CHECK(sym.getMember(), file->getFileName() +
+                                 ": could not get the member for symbol " +
+                                 toCOFFString(ctx, sym));
+  MemoryBufferRef mb =
+      CHECK(c.getMemoryBufferRef(),
+            file->getFileName() +
+                ": could not get the buffer for a child buffer of the archive");
+  if (identify_magic(mb.getBuffer()) == file_magic::coff_import_library) {
+    if (ctx.config.warnExportedDllMain) {
+      // We won't place DllMain symbols in the symbol table if they are
+      // coming from a import library. This message can be ignored with the flag
+      // '/ignore:exporteddllmain'
+      Warn(ctx)
+          << file->getFileName()
+          << ": skipping exported DllMain symbol [exporteddllmain]\nNOTE: this "
+             "might be a mistake when the DLL/library was produced.";
+    }
+    skipDllMain = true;
+    return true;
+  }
+  return false;
+}
+
 ArchiveFile::ArchiveFile(COFFLinkerContext &ctx, MemoryBufferRef m)
     : InputFile(ctx.symtab, ArchiveKind, m) {}
 
@@ -176,8 +205,17 @@ void ArchiveFile::parse() {
   }
 
   // Read the symbol table to construct Lazy objects.
-  for (const Archive::Symbol &sym : file->symbols())
+  bool skipDllMain = false;
+  for (const Archive::Symbol &sym : file->symbols()) {
+    // If the DllMain symbol was exported by mistake, skip importing it
+    // otherwise we might end up with a import thunk in the final binary which
+    // is wrong.
+    if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain") {
+      if (fixupDllMain(ctx, file.get(), sym, skipDllMain))
+        continue;
+    }
     archiveSymtab->addLazyArchive(this, sym);
+  }
 }
 
 // Returns a buffer pointing to a member file containing a given symbol.
diff --git a/lld/test/COFF/exported-dllmain.test b/lld/test/COFF/exported-dllmain.test
new file mode 100644
index 0000000000000..fcf6ed1005379
--- /dev/null
+++ b/lld/test/COFF/exported-dllmain.test
@@ -0,0 +1,57 @@
+REQUIRES: x86
+RUN: split-file %s %t.dir && cd %t.dir
+
+RUN: llvm-mc -filetype=obj -triple=x86_64-windows a.s -o a.obj
+
+RUN: llvm-mc -filetype=obj -triple=x86_64-windows b1.s -o b1.obj
+RUN: llvm-mc -filetype=obj -triple=x86_64-windows b2.s -o b2.obj
+
+### This is the line where our problem occurs. Here, we export the DllMain symbol which shouldn't happen normally.
+RUN: lld-link b1.obj b2.obj -out:b.dll -dll -implib:b.lib -entry:DllMain -export:bar -export:DllMain
+
+RUN: llvm-mc -filetype=obj -triple=x86_64-windows c.s -o c.obj
+RUN: lld-link -lib c.obj -out:c.lib
+
+### Later, if b.lib is provided before other libs/objs that export DllMain statically, we previously were using the dllimported DllMain from b.lib, which is wrong.
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain 2>&1 | FileCheck -check-prefix=WARN %s
+RUN: lld-link a.obj b.lib c.lib -dll -out:out.dll -entry:DllMain -ignore:exporteddllmain 2>&1 | FileCheck -check-prefix=IGNORED --allow-empty %s
+RUN: llvm-objdump --private-headers -d out.dll | FileCheck -check-prefix=DISASM %s
+
+WARN: lld-link: warning: b.lib: skipping exported DllMain symbol [exporteddllmain]
+IGNORED-NOT: lld-link: warning: b.lib: skipping exported DllMain symbol [exporteddllmain]
+
+DISASM: The Import Tables:
+DISASM: DLL Name: b.dll
+DISASM-NOT: DllMain
+DISASM: bar
+DISASM: Disassembly of section .text:
+DISASM-EMPTY:
+DISASM:      b8 01 00 00 00               movl    $0x1, %eax
+DISASM-NEXT: c3                           retq
+
+#--- a.s
+        .text
+        .globl foo
+foo:
+        call *__imp_bar(%rip)
+        ret
+
+#--- b1.s
+        .text
+        .globl bar
+bar:
+        ret
+
+#--- b2.s
+        .text
+        .globl DllMain
+DllMain:
+        xor %eax, %eax
+        ret
+
+#--- c.s
+        .text
+        .globl DllMain
+DllMain:
+        movl $1, %eax
+        ret

Copy link
Collaborator

@rnk rnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@aganea aganea merged commit e63de82 into llvm:main Jul 2, 2025
11 checks passed
@aganea aganea deleted the fix/lld_exported_dllmain branch July 2, 2025 12:53
@aganea aganea added this to the LLVM 20.X Release milestone Jul 2, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Jul 2, 2025
@aganea
Copy link
Member Author

aganea commented Jul 2, 2025

/cherry-pick e63de82

@llvmbot
Copy link
Member

llvmbot commented Jul 2, 2025

Failed to cherry-pick: e63de82

https://github.com/llvm/llvm-project/actions/runs/16025725389

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

aganea added a commit to aganea/llvm-project that referenced this pull request Jul 2, 2025
…6610)

This is a workaround for
llvm#82050 by skipping the `DllMain` symbol if seen in aimport library. If this situation occurs, after this commit a warning will also be displayed. The warning can be silenced with `/ignore:exporteddllmain`
@aganea aganea removed this from the LLVM 20.X Release milestone Jul 2, 2025
// '/ignore:exporteddllmain'
Warn(ctx)
<< file->getFileName()
<< ": skipping exported DllMain symbol [exporteddllmain]\nNOTE: this "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I'm a little late to the party here, but I think the option naming (and likewise the internal variables, but they're less effort to change later) feels a bit backwards here.

From the point of view of this link, there's an imported DllMain that we're ignoring. (This import lib entry probably exists because some DLL erroneously does export it, yes, but from the point of view of this linking, the issue is about an imported DllMain, not an issue about ourselves exporting DllMain.)

for (const Archive::Symbol &sym : file->symbols()) {
// If the DllMain symbol was exported by mistake, skip importing it
// otherwise we might end up with a import thunk in the final binary which
// is wrong.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this comment a bit hand-wavy. Obviously, calling another DLL's DllMain rathern than our own is not going to be doing the right thing - it's not so much about an import thunk or so, right? So I'd rather just say this:

// If an import library provides the DllMain symbol, skip importing it, as we should be using our own DllMain, not another DLL's DllMain.

// If the DllMain symbol was exported by mistake, skip importing it
// otherwise we might end up with a import thunk in the final binary which
// is wrong.
if (sym.getName() == "__imp_DllMain" || sym.getName() == "DllMain") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be missing the relevant prefix handling for i386?

@aganea
Copy link
Member Author

aganea commented Jul 5, 2025

I opened #147152 to address your comments @mstorsjo.
If instead you'd prefer me to revert this commit, and re-open a new PR I can do that too.

@mstorsjo
Copy link
Member

mstorsjo commented Jul 5, 2025

I opened #147152 to address your comments @mstorsjo. If instead you'd prefer me to revert this commit, and re-open a new PR I can do that too.

Nah, no need to revert - your incremental fix looks mostly good, just one comment on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Triage
Development

Successfully merging this pull request may close these issues.

4 participants