Bug 213547 - Mk/Scripts/qa.sh - hard-coded LD_LIBRARY_PATH causes false positives
Summary: Mk/Scripts/qa.sh - hard-coded LD_LIBRARY_PATH causes false positives
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Ports Framework (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Port Management Team
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-16 20:06 UTC by John Hein
Modified: 2016-12-09 20:43 UTC (History)
4 users (show)

See Also:


Attachments
[patch] remove hard-coded lib path for stage-qa (608 bytes, text/plain)
2016-10-16 20:06 UTC, John Hein
jcfyecrayz: maintainer-approval? (portmgr)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description John Hein 2016-10-16 20:06:50 UTC
Created attachment 175836 [details]
[patch] remove hard-coded lib path for stage-qa

The hard-coded LD_LIBRARY_PATH=${LOCALBASE}/lib causes problems when a library is installed both by ports and base.

Just one example: devel/jsoncpp ...

====> Running Q/A tests (stage-qa)
Error: /usr/local/lib/libjsoncpp.so.1.7.7 is linked to /usr/local/lib/libc++.so.1 from devel/libc++ but it is not declared as a dependency
Warning: you need LIB_DEPENDS+=libc++.so:devel/libc++


I don't see a reason why it might be needed at the moment, so my first patch  attempt just removes it.
Comment 1 Mathieu Arnold freebsd_committer 2016-10-18 12:01:00 UTC
There must be some reason for it, I'm asking about it in bug #195203 to the patch's submitter.
Comment 2 Yuri Victorovich freebsd_committer 2016-10-18 17:30:17 UTC
The original procedure in question was:
> lst_immediate_deps() {
>    LD_LIBRARY_PATH=${LOCALBASE}/lib ldd -a "$1" | \
>        awk 'BEGIN {section=0}; /^\// {section++} /^[^\/]/ {if(section<=1) print}' | \
>        sed -e 's/.*=> //' -e 's/ .*//'
>}

The current lines (after Mathieu modification):
> $(LD_LIBRARY_PATH=${LOCALBASE}/lib ldd -a "${STAGEDIR}${file}" | \
>       awk '\
>       BEGIN {section=0}\
>       /^\// {section++}\
>       !/^\// && section<=1 && ($3 ~ "^'${PREFIX}'" || $3 ~ "^'${LOCALBASE}'") {print $3}')

lst_immediate_deps() finds all immediate dependencies of the given elf. LD_LIBRARY_PATH=${LOCALBASE}/lib was added to merely ensure ldd looks there. So if it looks there by default it shouldn't make any difference at all. Calling code was selecting only the libraries that are under ${LOCALBASE}. So, again, LD_LIBRARY_PATH shouldn't matter if ldd looks there by default.

How does the output of ldd command differ on your system with and without LD_LIBRARY_PATH=${LOCALBASE}/lib? Or does it differ?

On my system /usr/local/lib/libjsoncpp.so.1.7.7 only depends on the base shared libraries:
> $ ldd /usr/local/lib/libjsoncpp.so.1.7.7
> /usr/local/lib/libjsoncpp.so.1.7.7:
>         libthr.so.3 => /lib/libthr.so.3 (0x801639000)
>         libc++.so.1 => /usr/lib/libc++.so.1 (0x80185d000)
>         libcxxrt.so.1 => /lib/libcxxrt.so.1 (0x801b1c000)
>         libm.so.5 => /lib/libm.so.5 (0x801d39000)
>         libgcc_s.so.1 => /lib/libgcc_s.so.1 (0x801f62000)
>         libc.so.7 => /lib/libc.so.7 (0x800822000)
I believe the difference is caused by being built locally (portupgrade) vs. in poudriere. Environment, the presence of preinstalled packages, impacts this.

I think it prints this warning properly for you:
> Warning: you need LIB_DEPENDS+=libc++.so:devel/libc++

The real problem is the volatility of dependency being base vs. localbase triggered by local vs. poudriere build. I am not sure what the solution of this problem couldbe, b
ut it has nothing to do with LD_LIBRARY_PATH here.
Comment 3 John Hein 2016-10-18 21:11:13 UTC
(In reply to yuri from comment #2)

Thanks for providing the background for why LD_LIBRARY_PATH was added.

By the way, here's the difference (you should get the same if the libc++ port is installed):

% ldd /usr/local/lib/libjsoncpp.so > /tmp/base ; env LD_LIBRARY_PATH=/usr/local/lib ldd /usr/local/lib/libjsoncpp.so > /tmp/ports ; diff -u /tmp/base /tmp/ports
> --- /tmp/base 2016-10-18 14:50:37.247449000 -0600
> +++ /tmp/ports        2016-10-18 14:50:37.263341000 -0600
> @@ -1,7 +1,8 @@
>  /usr/local/lib/libjsoncpp.so:
>       libthr.so.3 => /lib/libthr.so.3 (0x801639000)
> -     libc++.so.1 => /usr/lib/libc++.so.1 (0x80185d000)
> -     libcxxrt.so.1 => /lib/libcxxrt.so.1 (0x801b1c000)
> -     libm.so.5 => /lib/libm.so.5 (0x801d39000)
> -     libgcc_s.so.1 => /lib/libgcc_s.so.1 (0x801f62000)
> +     libc++.so.1 => /usr/local/lib/libc++.so.1 (0x80185d000)
> +     libcxxrt.so.1 => /lib/libcxxrt.so.1 (0x801b1d000)
> +     libm.so.5 => /lib/libm.so.5 (0x801d3a000)
> +     libgcc_s.so.1 => /lib/libgcc_s.so.1 (0x801f63000)
>       libc.so.7 => /lib/libc.so.7 (0x800822000)
> +     librt.so.1 => /usr/lib/librt.so.1 (0x802171000)

Pretty straightforward... the path specified in LD_LIBRARY_PATH is searched first causing it to find /usr/local/lib/libc++.so.1 (and its dependencies) before /usr/lib/libc++.so.1

If we remove LD_LIBRARY_PATH from qa.sh, we'll just use the normal system search path... which is the right thing for stage-qa to do.  Since you said it was added just to get ldd to look in /usr/local/lib, then it's definitely not needed.  The system looks there by default already (try 'ldconfig -r | grep search').  rpath and setuid-ness can change the lib search behavior a bit, but neither of those are involved in this case.

This same problem can happen whether you build with poudriere or not.  It may be that if you build jsoncpp using poudriere, devel/libc++ might not be installed in your jail.  In that case, of course you won't see the problem.  I believe you could try doing a bulk poudriere build with devel/libc++ and devel/jsoncpp if you really want to see the problem in poudriere.

In any case, inside or outside the jail, I believe it's safe (and correct) to remove the LD_LIBRARY_PATH.
Comment 4 Yuri Victorovich freebsd_committer 2016-10-18 21:19:34 UTC
I see. Then your patch is correct. Just commit it.
Comment 5 Yuri Victorovich freebsd_committer 2016-10-18 21:24:07 UTC
I don't have /usr/local/lib/libc++.so.1 installed.

Shared libs can be linked by the full path, or by the name alone. Not sure how this is chosen during build, but in the former case the preinstalled libs may influence the path it is linked with. That's how, I think, volatility comes into being. I can't recall which package I observed this on.
Comment 6 John Hein 2016-10-19 02:10:18 UTC
(In reply to yuri from comment #5)

The actual library that is "chosen during the build" depends on -L, -rpath and (as you mentioned) possibly libraries specified with an absolute path.

Since we're talking about dynamic linking, however, that's only half of the "linking" picture.

At run time (or when you run ldd), the second part of the linking job is finished off and the library file that is chosen can be influenced by a number of things, such as:

 - whether a run time "hint" was supplied at build time (-rpath)

 - LD_PRELOAD & LD_LIBRARY_PATH (if not setuid binaries unless you're root)

 - the ldconfig system search path


With all the variables involved, there is more than one way to fool stage-qa so that it doesn't necessarily find undocumented library dependencies.  Given the default system configuration, removing LD_LIBRARY_PATH from qa.sh removes one reasonably likely potential false positive source, and leaving it there doesn't help find undeclared dependencies at all.
Comment 7 Yuri Victorovich freebsd_committer 2016-10-19 02:15:51 UTC
Agreed!
Comment 8 commit-hook freebsd_committer 2016-10-25 11:45:00 UTC
A commit references this bug:

Author: mat
Date: Tue Oct 25 11:44:19 UTC 2016
New revision: 424616
URL: https://svnweb.freebsd.org/changeset/ports/424616

Log:
  Do not force a lookup in LOCALBASE/lib for shared libraries.

  ldd should work correctly if rpath is set, and this adds false
  positives.

  PR:		213547
  Submitted by:	John Hein
  Sponsored by:	Absolight

Changes:
  head/Mk/Scripts/qa.sh
Comment 9 John Hein 2016-12-09 20:43:45 UTC
r424616 fixed this but didn't automatically close the PR.
Closing it now.