Introduce a flag to warn when ifunc symbols are used with text relocations.

[tamur]

Summary

This patch adds a new flag, --warn-ifunc-textrel, to work around a glibc bug. When a code with ifunc symbols is used to produce an object file with text relocations, lld always succeeds. However, if that object file is linked using an old version of glibc, the resultant binary just crashes with segmentation fault when it is run (The bug is going to be corrected as of glibc 2.19).

Since there is no way to tell beforehand what library the object file will be linked against in the future, there does not seem to be a fool-proof way for lld to give an error only in cases where the binary will crash. So, with this change (dated 2018-09-25), lld starts to give a warning, contingent on a new command line flag that does not have a gnu counter part. The default value for --warn-ifunc-textrel is false, so lld behaviour will not change unless the user explicitly asks lld to give a warning. Users that link with a glibc library with version 2.19 or newer, or does not use ifunc symbols, or does not generate object files with text relocations do not need to take any action. Other users may consider to start passing warn-ifunc-textrel to lld to get early warnings.

[ruiu]

First line of a commit message should be shorter. Ideally less than 80 characters.

[ruiu]

ELF/Driver.cpp

832

A new variable name need to be consistent with other variables around it. In this case, all variable names are named after their corresponding command line flags, and the capitalization rule is to make a letter after "_" uppercase and then remove "_".

ELF/Relocations.cpp

986

Error messages need to be consistent -- please look other code when you are writing new code, so that new code looks the same as the other code. All other error messages start with a lowercase letter, and if they are followed by a source location, they end with ":".

[ruiu]

Remove "=" from the beginning of the first line of the commit message.

Please make it clear that you are proposing a new command line flag.

ELF/Driver.cpp

832

IFunc -> Ifunc

by that rule.

[ruiu]

Can you update the commit message to make it clear that this patch adds a new command line flag? That is very important information that cannot be omit from the commit log.

ELF/Options.td

352

Isn't IFUNC all uppercase?

[MaskRay]

test/ELF/textrel.s

36

If you rename main to _start, --entry=main on the link command line can be removed.

[MaskRay]

test/ELF/textrel.s

19

I find that in existing --fatal-warnings tests, 2>&1 is used only when the output will be piped to FileCheck. 2>&1 may be removed here.

[ruiu]

I don't see any commit message changes on https://reviews.llvm.org/D52430. Perhaps you need to manually copy-and-paste your commit message via the website.

[tamur]

Sorry, I thought I had updated the commit message. I hope it is done this time. Please take another look.

[ruiu]

Can you copy-n-paste your commit message here? https://reviews.llvm.org/differential/revision/edit/52430/

[MaskRay]

test/ELF/textrel.s

14

--check-prefix=error inspects CHECK-error: (missing in this file). I think the next line should be changed. It is also conventional to use an uppercase name.

[grimar]

New options should be mentioned in doc, see:
/lld/trunk/docs/ld.lld.1

ELF/Config.h

180

Flags here are alphabetically sorted, so WarnIfuncTextrel should be before the WarnMissingEntry I think.

test/ELF/textrel.s

4

ld -> lld

15

I think we need to check the full error message. Including the location it prints.

17

The same.

[ruiu]

Please separate your commit message into multiple paragraphs. I'd start a new line before "So, with this change".

Also I'd mention that "This patch adds a new flag, --warn-ifunc-textrel, to work around a nasty glibc bug." as the first sentence of the commit message.

ELF/Relocations.cpp

986

Please add the latest version of glibc and today's date along with a link to the bug management system's entry for the glibc crash issue as a comment, so that we can easily check whether a bug is fixed or not in the future. This flag is temporary; this is a workaround for the nasty glibc bug. We want to remove the flag within perhaps 5 years.

[MaskRay]

ELF/Relocations.cpp

986

The incompatibility issue has been fixed today (2018-09-25)
https://sourceware.org/bugzilla/show_bug.cgi?id=20480#c19

The next release glibc 2.29 will include the patch and run such executable fine.

There might be other security layers that block such executable from running (I am not sure if selinux can do such things) but they are not things linkers should consider.

You may consider changing the warning a bit to include the version information if this change is considered helpful. (It assuredly takes a long time for Linux distributions to adopt newer versions of glibc)

[ruiu]

The incompatibility issue has been fixed today (2018-09-25)
https://sourceware.org/bugzilla/show_bug.cgi?id=20480#c19

The next release glibc 2.29 will include the patch and run such executable fine.

Nice! It is definitely useful to include the version number in the warning; we should mention that glibc 2.28 or earlier has the bug.

[ruiu]

ELF/Relocations.cpp

988

Does recompiling without -fPIC really fix the issue?

[tamur]

ELF/Relocations.cpp

988

Linking without -fPIC and without -z,notext fixes the issue.

[ruiu]

That error message is displayed in one line, and usually error messages are not that long. And still the error message does not contain enough information about the warning. The other way to print out that error message is to make it more brief and include a URL to explain the issue, like this:

error: IFUNC symbols and text relocations are not allowed. For more info, see http://lld.llvm.org/ifunc-textrel

I feel this is better than a long error message, as we can explain the issue on the website.

What do you think? What other people think?

ELF/Relocations.cpp

988

"glibc 2.28 or earlier" is perhaps better because glibc 2.29 doesn't exit yet.

docs/ld.lld.1

432 ↗

(On Diff #167009)

using
IFUNC

[tamur]

The other way to print out that error message is to make it more brief and include a URL to explain the issue, like this:

error: IFUNC symbols and text relocations are not allowed. For more info, see http://lld.llvm.org/ifunc-textrel

I have no opinion whether this would be a good idea, but looking at the web site, there does not seem to be any precedent?

[grimar]

In D52430#1245927, @tamur wrote:

The other way to print out that error message is to make it more brief and include a URL to explain the issue, like this:

error: IFUNC symbols and text relocations are not allowed. For more info, see http://lld.llvm.org/ifunc-textrel

I have no opinion whether this would be a good idea, but looking at the web site, there does not seem to be any precedent?

I believe there was no any precedent yet.

In D52430#1245899, @ruiu wrote:

That error message is displayed in one line, and usually error messages are not that long. And still the error message does not contain enough information about the warning. The other way to print out that error message is to make it more brief and include a URL to explain the issue, like this:

error: IFUNC symbols and text relocations are not allowed. For more info, see http://lld.llvm.org/ifunc-textrel

I feel this is better than a long error message, as we can explain the issue on the website.

What do you think? What other people think?

The benefit from this that we can track and update the status of the fix and even maybe provide info about the known distributions affected/fixed
on the page what can be helpful for removing this flag at some longer-term point.

At the same time, I am not sure if we have other unobvious errors that might need the additional explanation. I guess this will be the only error referring to some URL,
what looks a bit inconsistent.

Another point is that perhaps it is the first flag we introduce knowing we will want to remove it. And perhaps having a page saying "it was introduced in LLD vX and removed in vY"
would be useful at some point.

I am not sure how the transaction (the removing of the flag) will happen though. There is nothing more permanent than temporary and simple
removing the flag can break some builds I think. Though forcing doing that would be the right thing still.

Given all the above I think having URL where we can post status updates is at least sound reasonable. So I see no issues having such error message.

[emaste]

The benefit from this that we can track and update the status of the fix and even maybe provide info about the known distributions affected/fixed on the page what can be helpful for removing this flag at some longer-term point.

I prefer to have all documentation self-contained rather than relying on an external resource - we could put the information in the man page description for --warn-ifunc-textrel. As we already know when the glibc fix is available (upstream at least) we wouldn't have anything to keep up-to-date: we can simply describe the issue, the fixed glibc version, and note that the option will be removed in the future. Keeping track of which distros have which glibc versions seems like a lot of work.

That said, this issue doesn't affect the BSDs, so you ought to proceed with whatever is most natural/useful for Linux folks.

docs/ld.lld.1

433 ↗

(On Diff #167026)

segmentation faults

[grimar]

In D52430#1246570, @emaste wrote:

The benefit from this that we can track and update the status of the fix and even maybe provide info about the known distributions affected/fixed on the page what can be helpful for removing this flag at some longer-term point.

I prefer to have all documentation self-contained rather than relying on an external resource - we could put the information in the man page description for --warn-ifunc-textrel. As we already know when the glibc fix is available (upstream at least) we wouldn't have anything to keep up-to-date: we can simply describe the issue, the fixed glibc version, and note that the option will be removed in the future.

Putting the information in the man page description for --warn-ifunc-textrel sound good.

Keeping track of which distros have which glibc versions seems like a lot of work.

Probably. We'll need this information though I think. Otherwise, how will we find that it is safe to remove the flag?

[emaste]

ELF/Options.td

352

I don't think so - GNU documentation uses lowercase ifunc in listing the attribute etc., I think the only uppercase cases are things like STT_GNU_IFUNC .

[emaste]

Probably. We'll need this information though I think. Otherwise, how will we find that it is safe to remove the flag?

Right. I just mean keeping a list up-to-date sounds like a lot of work - if we create such a list now we'd want to keep it updated. If we just say something like "will be removed in the future" we can wait some time, and confirm then that all relevant distros have new enough glibc.

[MaskRay]

In D52430#1246570, @emaste wrote:

The benefit from this that we can track and update the status of the fix and even maybe provide info about the known distributions affected/fixed on the page what can be helpful for removing this flag at some longer-term point.

I prefer to have all documentation self-contained rather than relying on an external resource - we could put the information in the man page description for --warn-ifunc-textrel. As we already know when the glibc fix is available (upstream at least) we wouldn't have anything to keep up-to-date: we can simply describe the issue, the fixed glibc version, and note that the option will be removed in the future. Keeping track of which distros have which glibc versions seems like a lot of work.

That said, this issue doesn't affect the BSDs, so you ought to proceed with whatever is most natural/useful for Linux folks.

I am sure IFUNC+DT_TEXTREL does not affect FreeBSD, but OpenBSD can be affected. Many other systems (including musl) may not support IFUNC at all.

[tamur]

I rewrote the man page, as it seems to be the consensus. I used lower case letters (ifunc) in the man page, but did not change the commit message. Please take another look.

[emaste]

I am sure IFUNC+DT_TEXTREL does not affect FreeBSD, but OpenBSD can be affected. Many other systems (including musl) may not support IFUNC at all.

Sure, but then this warning is irrelevant to them. I guess I should say "whatever is natural for glibc-using folks".

[ruiu]

Looks like this patch lacks a change you made to Options.td.

I'm sorry to be nitpicky, but maybe you should take a bit more time to review your code yourself, so that I don't have. :)

ELF/Relocations.cpp

990

Please be consistent -- you do not -fPIC with single quotes, so you shouldn't do that for -Wl,-z,-notext.

docs/ld.lld.1

433 ↗

(On Diff #167187)

2.28 and before -> 2.28 or earlier

Remove "," after ")".

440 ↗

(On Diff #167187)

Remove the trailing whitespace.

[tamur]

I did s/IFUNC/ifunc on all files and comments.

[ruiu]

ELF/Relocations.cpp

987

Indentation of this and all the following lines in this parentheses are off by one. Sorry to be nitpicky, but details matter, so please take time before sending a patch for review to fix this kind of errors.

[tamur]

ELF/Relocations.cpp

987

Thank you for your patience. I'm doing my best.

[ruiu]

LGTM

ELF/Config.h

struct Configuration {
   bool ThinLTOEmitImportsFiles;
   bool ThinLTOIndexOnly;
   bool UndefinedVersion;
   bool UseAndroidRelrTags = false;
   bool WarnBackrefs;
   bool WarnCommon;
   bool WarnMissingEntry;
   bool WarnSymbolOrdering;
+  bool WarnIfuncTextrel;
   bool WriteAddends;
   bool ZCombreloc;
   bool ZCopyreloc;
   bool ZExecstack;
   bool ZGlobal;
   bool ZHazardplt;
   bool ZInitfirst;
   bool ZInterpose;

ELF/Driver.cpp

void LinkerDriver::readConfigs(opt::InputArgList &Args) {
   Config->UndefinedVersion =
       Args.hasFlag(OPT_undefined_version, OPT_no_undefined_version, true);
   Config->UseAndroidRelrTags = Args.hasFlag(
       OPT_use_android_relr_tags, OPT_no_use_android_relr_tags, false);
   Config->UnresolvedSymbols = getUnresolvedSymbolPolicy(Args);
   Config->WarnBackrefs =
       Args.hasFlag(OPT_warn_backrefs, OPT_no_warn_backrefs, false);
   Config->WarnCommon = Args.hasFlag(OPT_warn_common, OPT_no_warn_common, false);
+  Config->WarnIfuncTextrel =
+      Args.hasFlag(OPT_warn_ifunc_textrel, OPT_no_warn_ifunc_textrel, false);
   Config->WarnSymbolOrdering =
       Args.hasFlag(OPT_warn_symbol_ordering, OPT_no_warn_symbol_ordering, true);
   Config->ZCombreloc = getZFlag(Args, "combreloc", "nocombreloc", true);
   Config->ZCopyreloc = getZFlag(Args, "copyreloc", "nocopyreloc", true);
   Config->ZExecstack = getZFlag(Args, "execstack", "noexecstack", false);
   Config->ZGlobal = hasZOption(Args, "global");
   Config->ZHazardplt = hasZOption(Args, "hazardplt");
   Config->ZInitfirst = hasZOption(Args, "initfirst");

ELF/Options.td

 defm warn_backrefs: B<"warn-backrefs",
     "Warn about backward symbol references to fetch archive members",
     "Do not warn about backward symbol references to fetch archive members (default)">;
 
 defm warn_common: B<"warn-common",
     "Warn about duplicate common symbols",
     "Do not warn about duplicate common symbols (default)">;
 
+defm warn_ifunc_textrel: B<"warn-ifunc-textrel",
+   "Warn about using IFUNC symbols with text relocations",
+   "Do not warn about using IFUNC symbols with text relocations (default)">;
+
 defm warn_symbol_ordering: B<"warn-symbol-ordering",
     "Warn about problems with the symbol ordering file (default)",
     "Do not warn about problems with the symbol ordering file">;
 
 def warn_unresolved_symbols: F<"warn-unresolved-symbols">,
   HelpText<"Report unresolved symbols as warnings">;
 
 defm whole_archive: B<"whole-archive",

ELF/Relocations.cpp

 #include "Config.h"
 #include "LinkerScript.h"
 #include "OutputSections.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "SyntheticSections.h"
 #include "Target.h"
 #include "Thunks.h"
+#include "lld/Common/ErrorHandler.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Strings.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 
 using namespace llvm;
static void scanReloc(InputSectionBase &Sec, OffsetGetter &GetOffset, RelTy *&I,
   // are determined by runtime.
   //
   // On the other hand, if we know that a PLT entry will be resolved within
   // the same ELF module, we can skip PLT access and directly jump to the
   // destination function. For example, if we are linking a main exectuable,
   // all dynamic symbols that can be resolved within the executable will
   // actually be resolved that way at runtime, because the main exectuable
   // is always at the beginning of a search list. We can leverage that fact.
-  if (Sym.isGnuIFunc())
+  if (Sym.isGnuIFunc()) {
+    if (!Config->ZText && Config->WarnIfuncTextrel) {
+      warn("using IFUNC symbols when text relocations are allowed may produce "
+          "a binary that will segfault, if the object file is linked with "
+          "glibc. Consider recompiling the object files without -fPIC and "
+          "without '-Wl,-z,notext' option: " + getLocation(Sec, Sym, Offset));
+    }
     Expr = toPlt(Expr);
-  else if (!Sym.IsPreemptible && Expr == R_GOT_PC && !isAbsoluteValue(Sym))
+  } else if (!Sym.IsPreemptible && Expr == R_GOT_PC && !isAbsoluteValue(Sym)) {
     Expr = Target->adjustRelaxExpr(Type, RelocatedAddr, Expr);
-  else if (!Sym.IsPreemptible)
+  } else if (!Sym.IsPreemptible) {
     Expr = fromPlt(Expr);
+  }
 
   // This relocation does not require got entry, but it is relative to got and
   // needs it to be created. Here we request for that.
   if (isRelExprOneOf<R_GOTONLY_PC, R_GOTONLY_PC_FROM_END, R_GOTREL,
                      R_GOTREL_FROM_END, R_PPC_TOC>(Expr))
     InX::Got->HasGotOffRel = true;
 
   // Read an addend.

test/ELF/textrel.s

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux-gnu %s -o %t.o
+
+# Without --warn-text-ifunc, ld should run fine:
+# RUN: ld.lld -z notext %t.o -o %t2
+
+# With --warn-text-ifunc, ld should run with warnings:
+# RUN: ld.lld --warn-ifunc-textrel -z notext %t.o -o /dev/null 2>&1
+#   | FileCheck %s --check-prefix=warning
+# CHECK: warning: Using IFUNC symbols when text relocations are allowed may
+
+# With --fatal-warnings, ld should fail with the error message:
+# RUN: not ld.lld --warn-ifunc-textrel --fatal-warnings -z notext
+#   %t.o -o /dev/null 2>&1 | FileCheck %s --check-prefix=error
+# CHECK: error: Using IFUNC symbols when text relocations are allowed may
+
+# Without text relocations, ld should run fine:
+# RUN: ld.lld --fatal-warnings %t.o -o /dev/null
+
+.text
+.globl a_func_impl
+a_func_impl:
+  nop
+
+.globl selector
+.type selector,@function
+selector:
+  movl $a_func_impl, %eax
+  retq
+
+.globl a_func
+.type a_func,@gnu_indirect_function
+.set a_func, selector
+
+.globl _start
+.type _start,@function
+main:
+  callq a_func
+  retq