#886 Enable BRP for detecting RPATH
Opened 5 years ago by churchyard. Modified 2 years ago

It would be nice if we detect RPATH in a brp script and fail the build if we do. Of course an opt-out would still be possible.

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_beware_of_rpath

Basically add the mentioned check-rpaths to %__arch_install_post or %__os_install_post.

WDYT?


I'm surprised that we don't have that yet. I'm definitely +1 to enable it..

The python3 package has a 00001-rpath.patch which removes rpath when Python builds a C extension, but only if the rpath is equal to /usr/lib64 (equal to Python LIBDIR sysconfig variable which is /usr/lib64 in practice). I proposed this change upstream, but after I talked with different people, I consider that this patch should not be made upstream, and that we should remove the downstream patch (from Fedora Python package).

Read https://bugs.python.org/issue36659 for more information.

The idea would be to first make sure that the build of a (Python) package fails if it uses rpath. Once redhat-rpm-macros https://src.fedoraproject.org/rpms/redhat-rpm-config/ ("macros" file) is updated, we will be able to remove 00001-rpath.patch from the Python package.

Also, note that:

There is a tool called check-rpaths which is included in the rpmdevtools package

Is probably no longer true:

$ rpm -qf /usr/lib/rpm/check-rpaths*
rpm-build-4.14.2.1-4.fc30.1.x86_64
rpm-build-4.14.2.1-4.fc30.1.x86_64

Metadata Update from @churchyard:
- Issue tagged with: meeting

5 years ago

We talked about this in this week meeting (https://meetbot-raw.fedoraproject.org/fedora-meeting-1/2019-05-16/fpc.2019-05-16-16.00.txt):

  • #886 Enable BRP for detecting RPATH (geppetto, 16:24:31)
  • ACTION: Enable BRP for detecting RPATH (+1:6, 0:0, -1:0) (geppetto,
    16:39:17)

Metadata Update from @james:
- Issue untagged with: meeting
- Issue tagged with: writeup

5 years ago

So this now needs two things:

  • A rewrite of the "Beware of RPATH" section (which was needed even before this).

  • A special announcement about what this will break and providing a way to work around it.

  • Adding a definition of %__brp_check_rpaths, along with adding %{?__brp_check_rpaths} to the definition of %__os_install_post in the redhat-rpm-config package.

Concerns I have:

  • How many potential failures there could be. Do we have any idea? There are more than a few packages which trip rpmlint's binary-or-shlib-defines-rpath complaint, and my understanding is that these will indeed cause check-rpaths to fail (with "contains a standard rpath").

  • Pushback if the failure count is high.

  • check-rpath failures will be oddly verbose and contain information about setting a magic environment variable QA_RPATHS. This information is actually correct as far as I know, though it doesn't tell you that you have to do that in %install. It's also a bit odd because nothing else in the system seems to work that way. But the script comes from RPM upstream so I guess that's the way it is.

I'm thinking that maybe this should go through the actual change process (with our approval on it and the guidelines changes ready to go, of course). I'm still for it but... maybe this isn't really something FPC should just do.

I'm thinking that maybe this should go through the actual change process

That has been on my mind as well. I think we should do this as a Fedora change. It has all the benefits, the thing is:

  • documented
  • discussed
  • approved
  • announced
  • tracked

For the record, I ran rpmlint -c BinariesCheck on all of the x86_64 packages in the most recent rawhide compose and pulled out the "binary-or-shlib-defines-rpath" complaints. There are 365. Some of these use rpaths like $ORIGIN/. which I believe check-rpaths should allow.

A few packages, like condor, will run afoul of the check that $ORIGIN rpaths must come first (and also the issue of including the stock libary path):

condor.x86_64: E: binary-or-shlib-defines-rpath /usr/libexec/condor/accountant_log_fixer ['$ORIGIN/../lib', '/lib64', '/usr/lib64', '$ORIGIN/../lib/condor', '/usr/lib64/condor']

This one is fun (and seems to me to be rather broken):

esc.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/esc-1.1.2/lib/libcoolkeymgr-1.0.so.0.0.0 ['/usr/local/lib']

This seems pretty broken as well; shared libraries in /usr/share?

conky-manager.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/conky-manager ['/usr/share/conky-manager/libs']

These are unpleasant:

Io-language.x86_64: E: binary-or-shlib-defines-rpath /usr/lib64/io/addons/LZO/_build/dll/libIoLZO.so ['/builddir/build/BUILD/io-e64ff9/_build/dll']
jq.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/jq ['/builddir/build/BUILD/jq-1.6/.libs', '/usr/lib64']

And this is wrong on a couple of levels:

libp11-devel.x86_64: E: binary-or-shlib-defines-rpath /usr/share/doc/libp11-devel/examples/.libs/lt-auth ['/builddir/build/BUILD/libp11-0.4.10/src/.libs']

But by far the biggest thing is packages with an rpath of /usr/lib64, which I believe will be failed by check-rpaths. There are over 300 of these and include some pretty core package like acl, attr, binutils.

Doing this has left me with some questions, like whether this is OK:

lldb.x86_64: E: binary-or-shlib-defines-rpath /usr/bin/lldb ['$ORIGIN/../lib64']

Since the binary is in /usr/bin, that is no different than an rpath of /usr/lib64. But don't believe check-rpaths will complain. And if that's OK, then... why does it complain about /usr/lib64?

If things were patched to not complain (or maybe just warn) about standard rpaths by default then I would think this would catch some pretty broken stuff while not causing too much disruption. Maybe that's a better place to start, but unfortunately the script comes from RPM upstream which means that changing it can be somewhat difficult. I think we might get by with some trickery like setting %__brp_check_rpaths to

QA_RPATHS=${QA_RPATHS-1} /usr/lib/rpm/check-rpaths

but I'd have to try it to see and I'm sure that someone would yell about the bashism.

Anyway, I guess I'll start a discussion on the devel list.

So I took a look at it, to try to assess the impact and maybe propose a system-wide change for F35 to enable rpath detection (and failure of those builds) by default.

Currently in my analysis of all the x86_64 packages, 92 fail.

Methodology used for analysis:

Repoquery all the x86_64 packages in rawhide which yielded 9514 items.

Build them in copr with this change in redhat-rpm-config:

diff --git a/macros b/macros
index f062f94..b5ccb07 100644
--- a/macros
+++ b/macros
@@ -234,7 +234,7 @@ print(result)
 #      Expanded at end of %install scriptlet.
 #

-%__arch_install_post   /usr/lib/rpm/check-buildroot
+%__arch_install_post  /usr/lib/rpm/check-rpaths  /usr/lib/rpm/check-buildroot

Copr repo: https://copr.fedorainfracloud.org/coprs/cstratak/rpath/

And then analyze all the failures through a script with this regex:
.+'check-rpaths' detected a broken RPATH.+

@tibbs Do you see any blockers in moving this forward? It has already been discussed and the consensus is that this should move forward, so I'd be happy to work on it. Does anyone else has any feedback?

  1. Could you share the full list?

  2. There must be a way to opt out. The script currently says:

* Examples:
* - to ignore standard and empty RPATHs, execute 'rpmbuild' like
*   $ QA_RPATHS=$(( 0x0001|0x0010 )) rpmbuild my-package.src.rpm

This is impossible in mock, koji etc. It should follow standard disablement described in https://docs.fedoraproject.org/en-US/packaging-guidelines/#_brp_buildroot_policy_scripts + additionally it could allow setting a macro with the specific errors to ignore. E.g. %global %_brp_rpath_allow 0x0001|0x0010

  1. I believe you accidentally disabled /usr/lib/rpm/check-buildroot by moving it to an argument of /usr/lib/rpm/check-rpaths. For the purpose of your check, it doesn't really matter, but when you do the change, please make sure both are executed.
  1. Could you share the full list?

Sure. I'll add it to a subsequent comment as to not make the comment too long.

  1. There must be a way to opt out. The script currently says:

* Examples: * - to ignore standard and empty RPATHs, execute 'rpmbuild' like * $ QA_RPATHS=$(( 0x0001|0x0010 )) rpmbuild my-package.src.rpm

This is impossible in mock, koji etc. It should follow standard disablement described in https://docs.fedoraproject.org/en-US/packaging-guidelines/#_brp_buildroot_policy_scripts + additionally it could allow setting a macro with the specific errors to ignore. E.g. %global %_brp_rpath_allow 0x0001|0x0010

Indeed there should be a proper opt-out mechanism. I haven't implemented anything of the sort yet, but if it moves to a system-wide change that will be included.

  1. I believe you accidentally disabled /usr/lib/rpm/check-buildroot by moving it to an argument of /usr/lib/rpm/check-rpaths. For the purpose of your check, it doesn't really matter, but when you do the change, please make sure both are executed.

Thanks for the heads-up!

Full list of packages:

audiofile
abc
binutils
esc
ettercap
freeradius
fortune-mod
fcl
eb
conky-manager
condor
community-mysql
czmq
cfitsio
compat-guile18
glib2
gnokii
koffice-kivio
kicad
jq
komparator
k3guitune
laszip
levmar
hdf
gpick
kdepim3
gpgme
Io-language
kdegames3
gupnp-dlna
kdebase3
libcommuni
lutok
libburn
libminc
libisoburn
liboping
librfid
mingw-qt5-qtdeclarative
libkkc
openjade
libdxfrw
libosip2
libeXosip2
NLopt
libprelude
mingw-qt5-qt3d
mod_wsgi
libXcm
ncview
libdkimpp
mingw-qt5-qttools
mcpp
mingw-qt5-qtbase
mongo-c-driver
nightview
openscap
plotmm
pam_yubico
perl-SDL
pinentry
pam_mount
python2.7
rb_libtorrent
rrdtool
rarian
qwtpolar
qucs
scipy
tracker
SDL_image
sofia-sip
scap-workbench
woff2
xeus
yaz
stp
suitesparse
usnic-tools
sqlite2
vanessa_logger
xbsql
tracker-miners
WindowMaker
xmms
sylfilter
verbiste
zvbi
xdotool
texlive-base
zinnia

I had completely forgotten about this effort. Thanks for picking it back up.

Honestly things don't look that bad; the package list seems smaller than it was when I looked two years ago. With the above-mentioned opt-out and tweaking options actually implemented, I don't see why the checking couldn't be enabled after the usual distro-wide coordination, assuming the RPM folks don't have any objections. It would, however, be good to make sure that we broadly understand what these packages are doing wrong, and what they should be doing to fix the issue instead of just disabling the check.

Once all of that is done, I suppose the actual changes to the guidelines would be pretty simple.

On a slightly related note, I see that the call to /usr/lib/rpm/check-buildroot in %__arch_install_post is also not conditionalized. It's kind of odd; I don't really understand the distinction between %__os_install_post and %__arch_install_post given that the latter only contains one thing and they are both just incorporated unconditionally into %__spec_install_post.

Seems to me that everything (including the change to call check-rpaths) should be uniformly conditionalized and placed in %__os_install_post for consistency.

Metadata Update from @tibbs:
- Issue untagged with: writeup
- Issue tagged with: meeting

3 years ago

Wow, congrats for your tenacity cstratak!

On a related note, maybe the guidelines will have to amended to account for the fact that the buildroot policy script was updated in RPM upstream to check and apply the same rules for RUNPATH also for RPM => 4.17, at least when RPM is updated.

https://github.com/rpm-software-management/rpm/pull/1487/files

Can this issue be closed, or was there something else that you wanted the FPC to discuss/decide? I'm moving the issue to needinfo because nothing has really happened in months, but it might be better to just close and reference it in a new issue if you have follow on questions/requests.

Metadata Update from @james:
- Issue untagged with: meeting
- Issue tagged with: needinfo

2 years ago

The BRP was enabled, but the guidelines wrt it are outdated.

Log in to comment on this ticket.

Metadata
Related Pull Requests
  • #1071 Last updated 2 years ago