This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by tamur on Sep 24 2018, 12:02 PM.

Details

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.

Event Timeline

tamur created this revision.Sep 24 2018, 12:02 PM
ruiu added a comment.Sep 24 2018, 12:25 PM

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

ruiu added inline comments.Sep 24 2018, 12:28 PM
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 ":".

emaste added a subscriber: markj.Sep 24 2018, 12:55 PM
tamur retitled this revision from =Issue a warning when IFUNC symbols are used and text relocations are allowed. This is because it produces a binary that seg faults when the object file is linked with glibc. The warning is contingent on a command line flag (default is false)... to =Issue a warning when IFUNC symbols are used and text relocations are allowed..Sep 24 2018, 1:35 PM
tamur edited the summary of this revision. (Show Details)
tamur updated this revision to Diff 166747.Sep 24 2018, 1:57 PM
tamur marked 2 inline comments as done.
ruiu added a comment.Sep 24 2018, 2:58 PM

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.

tamur updated this revision to Diff 166763.Sep 24 2018, 3:20 PM
tamur retitled this revision from =Issue a warning when IFUNC symbols are used and text relocations are allowed. to Introduce a flag to warn when IFUNC symbols are used with text relocations..
tamur edited the summary of this revision. (Show Details)
tamur marked an inline comment as done.Sep 24 2018, 3:23 PM
ruiu added a comment.Sep 24 2018, 3:27 PM

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?

tamur updated this revision to Diff 166776.Sep 24 2018, 3:56 PM
tamur marked an inline comment as done.
MaskRay added inline comments.
test/ELF/textrel.s
36

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

MaskRay added inline comments.Sep 24 2018, 4:35 PM
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.

tamur updated this revision to Diff 166785.Sep 24 2018, 4:42 PM
tamur marked 2 inline comments as done.
ruiu added a comment.Sep 24 2018, 5:02 PM

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 updated this revision to Diff 166792.Sep 24 2018, 5:31 PM

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

tamur updated this revision to Diff 166798.Sep 24 2018, 8:03 PM
tamur edited the summary of this revision. (Show Details)
MaskRay added inline comments.Sep 24 2018, 8:33 PM
test/ELF/textrel.s
15

--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 added a subscriber: grimar.Sep 25 2018, 2:35 AM

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

ELF/Config.h
181

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

test/ELF/textrel.s
5

ld -> lld

16

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

18

The same.

ruiu added a comment.Sep 25 2018, 5:06 AM

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 added inline comments.Sep 25 2018, 1:43 PM
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 added a comment.Sep 25 2018, 2:15 PM

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 added inline comments.Sep 25 2018, 2:24 PM
ELF/Relocations.cpp
988

Does recompiling without -fPIC really fix the issue?

tamur updated this revision to Diff 167009.Sep 25 2018, 3:29 PM
tamur edited the summary of this revision. (Show Details)
tamur marked 7 inline comments as done.Sep 25 2018, 3:32 PM
tamur added inline comments.
ELF/Relocations.cpp
988

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

ruiu added a comment.Sep 25 2018, 3:57 PM

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

using
IFUNC

tamur updated this revision to Diff 167026.Sep 25 2018, 4:48 PM
tamur edited the summary of this revision. (Show Details)
tamur marked an inline comment as done.Sep 25 2018, 4:51 PM

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 added a comment.EditedSep 26 2018, 2:11 AM

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.

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.

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

segmentation faults

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 added inline comments.Sep 26 2018, 7:22 AM
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 .

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.

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 updated this revision to Diff 167187.Sep 26 2018, 1:30 PM

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.

tamur marked 3 inline comments as done.Sep 26 2018, 1:31 PM

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 added a comment.Sep 27 2018, 11:36 AM

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

2.28 and before -> 2.28 or earlier

Remove "," after ")".

440

Remove the trailing whitespace.

tamur updated this revision to Diff 167399.Sep 27 2018, 1:59 PM
tamur retitled this revision from Introduce a flag to warn when IFUNC symbols are used with text relocations. to Introduce a flag to warn when ifunc symbols are used with text relocations..
tamur edited the summary of this revision. (Show Details)

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

tamur marked 3 inline comments as done.Sep 27 2018, 2:02 PM
ruiu added inline comments.Sep 27 2018, 2:41 PM
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 updated this revision to Diff 167414.Sep 27 2018, 4:45 PM
tamur marked an inline comment as done.
tamur added inline comments.
ELF/Relocations.cpp
987

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

ruiu accepted this revision.Sep 27 2018, 4:52 PM

LGTM

This revision is now accepted and ready to land.Sep 27 2018, 4:52 PM
This revision was automatically updated to reflect the committed changes.
tamur marked an inline comment as done.