[PATCH 0/2] RPATH host pollution fixes

[PATCH 0/2] RPATH host pollution fixes

[Andy Ross]

Results from chasing the RPATH pollution I was seeing:

I don't have a fix for the underlying linker issue, but that is
captured here:

  https://bugzilla.yoctoproject.org/show_bug.cgi?id=2965

Patch 1 is the warning fix from earlier, which is still a fix to the
existing logic and should be applied.  I've removed the
"host-polluted" text though, as AFAICT the actual RPATH is not a
problem, only the use of -rpath with the linker that produced it.

Patch 2 is a fix to libtool on top of the existing fix-rpath.patch to
normalize the strings.  With this applied, all the link failures we
have reproduced (including the one reported vs. owl_video last week)
work correctly.  No libraries in the resulting sysroot have a RPATH
set to a default link directory.

A few runnable binaries still trigger the warning though (but
otherwise build successfully), I'm looking at that now -- I suspect
these simply aren't using libtool, or else we've missed a spot where
normalization and/or default directory pruning needs to happen.

Andy

[PATCH 1/2] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Andy Ross]

In toolchain edge cases it's possible for the RPATH of a library to be
set to something like "/usr/lib/../lib".  This should be detected as
"/usr/lib" and generate a warning.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
 meta/classes/insane.bbclass | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 556a176..b84a89f 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -161,6 +161,10 @@ def package_qa_check_rpath(file,name, d, elf, messages):
             if dir in line:
                 messages.append("package %s contains bad RPATH %s in file %s" % (name, line, file))
 
+def rpath_eq(a, b):
+    import os.path
+    return os.path.normpath(a) == os.path.normpath(b)
+
 QAPATHTEST[useless-rpaths] = "package_qa_check_useless_rpaths"
 def package_qa_check_useless_rpaths(file, name, d, elf, messages):
     """
@@ -181,7 +185,7 @@ def package_qa_check_useless_rpaths(file, name, d, elf, messages):
     	m = rpath_re.match(line)
 	if m:
 	   rpath = m.group(1)
-	   if rpath == libdir or rpath == base_libdir:
+	   if rpath_eq(rpath, libdir) or rpath_eq(rpath, base_libdir):
 	      # The dynamic linker searches both these places anyway.  There is no point in
 	      # looking there again.
 	      messages.append("%s: %s contains probably-redundant RPATH %s" % (name, package_qa_clean_path(file, d), rpath))
-- 
1.7.11.2

[PATCH 2/2] libtool: normalize link paths before considering for RPATH

[Andy Ross]

Libtool may be passed link paths of the form "/usr/lib/../lib", which fool
its detection code into thinking it should be included as an RPATH in
the generated binary.  Normalize before comparision.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
 meta/recipes-devtools/libtool/libtool-2.4.2.inc    |  1 +
 .../libtool/libtool/norm-rpath.patch               | 42 ++++++++++++++++++++++
 2 files changed, 43 insertions(+)
 create mode 100644 meta/recipes-devtools/libtool/libtool/norm-rpath.patch

diff --git a/meta/recipes-devtools/libtool/libtool-2.4.2.inc b/meta/recipes-devtools/libtool/libtool-2.4.2.inc
index 5b9557e..691427e 100644
--- a/meta/recipes-devtools/libtool/libtool-2.4.2.inc
+++ b/meta/recipes-devtools/libtool/libtool-2.4.2.inc
@@ -19,6 +19,7 @@ SRC_URI = "${GNU_MIRROR}/libtool/libtool-${PV}.tar.gz \
            file://avoid_absolute_paths_for_general_utils.patch \
            file://fix-rpath.patch \
 	   file://respect-fstack-protector.patch \
+           file://norm-rpath.patch \
           "
 
 SRC_URI[md5sum] = "d2f3b7d4627e69e13514a40e72a24d50"
diff --git a/meta/recipes-devtools/libtool/libtool/norm-rpath.patch b/meta/recipes-devtools/libtool/libtool/norm-rpath.patch
new file mode 100644
index 0000000..03a7667
--- /dev/null
+++ b/meta/recipes-devtools/libtool/libtool/norm-rpath.patch
@@ -0,0 +1,42 @@
+libtool: normalize link paths before considering for RPATH
+
+Libtool may be passed link paths of the form "/usr/lib/../lib", which
+fool its detection code into thinking it should be included as an
+RPATH in the generated binary.  Normalize before comparision.
+
+Signed-off-by: Andy Ross <andy.ross@windriver.com>
+Upstream-Status: Pending
+
+diff -ru a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
+--- a/libltdl/config/ltmain.m4sh	2012-08-16 13:58:55.058900363 -0700
++++ b/libltdl/config/ltmain.m4sh	2012-08-16 16:34:54.616627821 -0700
+@@ -7288,8 +7288,13 @@
+ 	      else
+                 # We only want to hardcode in an rpath if it isn't in the
+                 # default dlsearch path.
++                libdir_norm=`echo $libdir \
++                  | sed 's/\/\+\.\(\/\+\|$\)/\//g' \
++                  | sed 's/[^\/]\+\/\+\.\.\(\/\+\|$\)//g' \
++                  | sed 's/\/\+/\//g' \
++                  | sed 's/\(.\)\/$/\1/g'`
+ 	        case " $sys_lib_dlsearch_path " in
+-	        *" $libdir "*) ;;
++	        *" $libdir_norm "*) ;;
+ 	        *) eval flag=\"$hardcode_libdir_flag_spec\"
+                    func_append dep_rpath " $flag"
+                    ;;
+@@ -8027,8 +8032,13 @@
+ 	  else
+             # We only want to hardcode in an rpath if it isn't in the
+             # default dlsearch path.
++            libdir_norm=`echo $libdir \
++              | sed 's/\/\+\.\(\/\+\|$\)/\//g' \
++              | sed 's/[^\/]\+\/\+\.\.\(\/\+\|$\)//g' \
++              | sed 's/\/\+/\//g' \
++              | sed 's/\(.\)\/$/\1/g'`
+ 	    case " $sys_lib_dlsearch_path " in
+-	    *" $libdir "*) ;;
++	    *" $libdir_norm "*) ;;
+ 	    *) eval flag=\"$hardcode_libdir_flag_spec\"
+                rpath+=" $flag"
+                ;;
-- 
1.7.11.2

Re: [PATCH 2/2] libtool: normalize link paths before considering for RPATH

[Richard Purdie]

On Fri, 2012-08-17 at 08:53 -0700, Andy Ross wrote:
> Libtool may be passed link paths of the form "/usr/lib/../lib", which fool
> its detection code into thinking it should be included as an RPATH in
> the generated binary.  Normalize before comparision.
> 
> Signed-off-by: Andy Ross <andy.ross@windriver.com>
> ---
>  meta/recipes-devtools/libtool/libtool-2.4.2.inc    |  1 +
>  .../libtool/libtool/norm-rpath.patch               | 42 ++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
>  create mode 100644 meta/recipes-devtools/libtool/libtool/norm-rpath.patch
> 
> diff --git a/meta/recipes-devtools/libtool/libtool-2.4.2.inc b/meta/recipes-devtools/libtool/libtool-2.4.2.inc
> index 5b9557e..691427e 100644
> --- a/meta/recipes-devtools/libtool/libtool-2.4.2.inc
> +++ b/meta/recipes-devtools/libtool/libtool-2.4.2.inc
> @@ -19,6 +19,7 @@ SRC_URI = "${GNU_MIRROR}/libtool/libtool-${PV}.tar.gz \
>             file://avoid_absolute_paths_for_general_utils.patch \
>             file://fix-rpath.patch \
>  	   file://respect-fstack-protector.patch \
> +           file://norm-rpath.patch \
>            "
>  
>  SRC_URI[md5sum] = "d2f3b7d4627e69e13514a40e72a24d50"
> diff --git a/meta/recipes-devtools/libtool/libtool/norm-rpath.patch b/meta/recipes-devtools/libtool/libtool/norm-rpath.patch
> new file mode 100644
> index 0000000..03a7667
> --- /dev/null
> +++ b/meta/recipes-devtools/libtool/libtool/norm-rpath.patch
> @@ -0,0 +1,42 @@
> +libtool: normalize link paths before considering for RPATH
> +
> +Libtool may be passed link paths of the form "/usr/lib/../lib", which
> +fool its detection code into thinking it should be included as an
> +RPATH in the generated binary.  Normalize before comparision.
> +
> +Signed-off-by: Andy Ross <andy.ross@windriver.com>
> +Upstream-Status: Pending
> +
> +diff -ru a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
> +--- a/libltdl/config/ltmain.m4sh	2012-08-16 13:58:55.058900363 -0700
> ++++ b/libltdl/config/ltmain.m4sh	2012-08-16 16:34:54.616627821 -0700
> +@@ -7288,8 +7288,13 @@
> + 	      else
> +                 # We only want to hardcode in an rpath if it isn't in the
> +                 # default dlsearch path.
> ++                libdir_norm=`echo $libdir \
> ++                  | sed 's/\/\+\.\(\/\+\|$\)/\//g' \
> ++                  | sed 's/[^\/]\+\/\+\.\.\(\/\+\|$\)//g' \
> ++                  | sed 's/\/\+/\//g' \
> ++                  | sed 's/\(.\)\/$/\1/g'`
> + 	        case " $sys_lib_dlsearch_path " in
> +-	        *" $libdir "*) ;;
> ++	        *" $libdir_norm "*) ;;
> + 	        *) eval flag=\"$hardcode_libdir_flag_spec\"
> +                    func_append dep_rpath " $flag"
> +                    ;;

Can't we use func_norm_abspath here?

Cheers,

Richard

Re: [PATCH 2/2] libtool: normalize link paths before considering for RPATH

[Andy Ross]

On 08/19/2012 03:06 AM, Richard Purdie wrote:
> On Fri, 2012-08-17 at 08:53 -0700, Andy Ross wrote:
>> ++                libdir_norm=`echo $libdir \
>> ++                  | sed 's/\/\+\.\(\/\+\|$\)/\//g' \
>> ++                  | sed 's/[^\/]\+\/\+\.\.\(\/\+\|$\)//g' \
>> ++                  | sed 's/\/\+/\//g' \
>> ++                  | sed 's/\(.\)\/$/\1/g'`
>
> Can't we use func_norm_abspath here?

I have to admit I got a little confused reading that code (not that my
sed mess is significantly better, but at least I trust it because I
wrote it); but it looks to me like it's an abspath implementation on
the host filesystem (not the use of `pwd` in a few places).  That will
work for pruning in this case, since the problem case is already an
absolute path to a host directory.  But I don't see how it won't in
principle break things by expanding host paths.

Andy

Re: [PATCH 2/2] libtool: normalize link paths before considering for RPATH

[Richard Purdie]

On Mon, 2012-08-20 at 09:59 -0700, Andy Ross wrote:
> On 08/19/2012 03:06 AM, Richard Purdie wrote:
> > On Fri, 2012-08-17 at 08:53 -0700, Andy Ross wrote:
> >> ++                libdir_norm=`echo $libdir \
> >> ++                  | sed 's/\/\+\.\(\/\+\|$\)/\//g' \
> >> ++                  | sed 's/[^\/]\+\/\+\.\.\(\/\+\|$\)//g' \
> >> ++                  | sed 's/\/\+/\//g' \
> >> ++                  | sed 's/\(.\)\/$/\1/g'`
> >
> > Can't we use func_norm_abspath here?
> 
> I have to admit I got a little confused reading that code (not that my
> sed mess is significantly better, but at least I trust it because I
> wrote it); but it looks to me like it's an abspath implementation on
> the host filesystem (not the use of `pwd` in a few places).  That will
> work for pruning in this case, since the problem case is already an
> absolute path to a host directory.  But I don't see how it won't in
> principle break things by expanding host paths.

As I read it, it will only use pwd if the path is relative or empty. I
can't think of a case where you'd encode something like that into an
rpath...

Libtool can't expect the directory to exist on the filesystem when it
makes these calls so we should be safe from that perspective.

Cheers,

Richard

Re: [PATCH 1/2] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Richard Purdie]

On Fri, 2012-08-17 at 08:53 -0700, Andy Ross wrote:
> In toolchain edge cases it's possible for the RPATH of a library to be
> set to something like "/usr/lib/../lib".  This should be detected as
> "/usr/lib" and generate a warning.
> 
> Signed-off-by: Andy Ross <andy.ross@windriver.com>
> ---
>  meta/classes/insane.bbclass | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 556a176..b84a89f 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -161,6 +161,10 @@ def package_qa_check_rpath(file,name, d, elf, messages):
>              if dir in line:
>                  messages.append("package %s contains bad RPATH %s in file %s" % (name, line, file))
>  
> +def rpath_eq(a, b):
> +    import os.path

You don't need that import, os is always assumed to be present for our
code since it gets used so often. I'd suggest defining this within the 
package_qa_check_useless_rpaths() function too since its slightly less
effort for the parser.

Cheers,

Richard

> +    return os.path.normpath(a) == os.path.normpath(b)
> +
>  QAPATHTEST[useless-rpaths] = "package_qa_check_useless_rpaths"
>  def package_qa_check_useless_rpaths(file, name, d, elf, messages):
>      """
> @@ -181,7 +185,7 @@ def package_qa_check_useless_rpaths(file, name, d, elf, messages):
>      	m = rpath_re.match(line)
>  	if m:
>  	   rpath = m.group(1)
> -	   if rpath == libdir or rpath == base_libdir:
> +	   if rpath_eq(rpath, libdir) or rpath_eq(rpath, base_libdir):
>  	      # The dynamic linker searches both these places anyway.  There is no point in
>  	      # looking there again.
>  	      messages.append("%s: %s contains probably-redundant RPATH %s" % (name, package_qa_clean_path(file, d), rpath))

[PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Andy Ross]

In toolchain edge cases it's possible for the RPATH of a library to be
set to something like "/usr/lib/../lib".  This should be detected as
"/usr/lib" and generate a warning.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
 meta/classes/insane.bbclass | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 556a176..9d085a4 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -166,6 +166,9 @@ def package_qa_check_useless_rpaths(file, name, d, elf, messages):
     """
     Check for RPATHs that are useless but not dangerous
     """
+    def rpath_eq(a, b):
+        return os.path.normpath(a) == os.path.normpath(b)
+
     if not elf:
         return
 
@@ -181,7 +184,7 @@ def package_qa_check_useless_rpaths(file, name, d, elf, messages):
     	m = rpath_re.match(line)
 	if m:
 	   rpath = m.group(1)
-	   if rpath == libdir or rpath == base_libdir:
+	   if rpath_eq(rpath, libdir) or rpath_eq(rpath, base_libdir):
 	      # The dynamic linker searches both these places anyway.  There is no point in
 	      # looking there again.
 	      messages.append("%s: %s contains probably-redundant RPATH %s" % (name, package_qa_clean_path(file, d), rpath))
-- 
1.7.11.2

Re: [PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Saul Wold]

On 08/20/2012 02:05 PM, Andy Ross wrote:
> In toolchain edge cases it's possible for the RPATH of a library to be
> set to something like "/usr/lib/../lib".  This should be detected as
> "/usr/lib" and generate a warning.
>
> Signed-off-by: Andy Ross <andy.ross@windriver.com>
> ---
>   meta/classes/insane.bbclass | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 556a176..9d085a4 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -166,6 +166,9 @@ def package_qa_check_useless_rpaths(file, name, d, elf, messages):
>       """
>       Check for RPATHs that are useless but not dangerous
>       """
> +    def rpath_eq(a, b):
> +        return os.path.normpath(a) == os.path.normpath(b)
> +
>       if not elf:
>           return
>
> @@ -181,7 +184,7 @@ def package_qa_check_useless_rpaths(file, name, d, elf, messages):
>       	m = rpath_re.match(line)
>   	if m:
>   	   rpath = m.group(1)
> -	   if rpath == libdir or rpath == base_libdir:
> +	   if rpath_eq(rpath, libdir) or rpath_eq(rpath, base_libdir):
>   	      # The dynamic linker searches both these places anyway.  There is no point in
>   	      # looking there again.
>   	      messages.append("%s: %s contains probably-redundant RPATH %s" % (name, package_qa_clean_path(file, d), rpath))
>

Merged this to OE-Core

Thanks
	Sau!

[PATCH] Fix RPATH warning vs. weird paths

[Andy Ross]

We hit some oddball cases where libraries were being built with RPATHs
referencing host directores and causing version skew in builds.  There
is a warning case to detect that already, but it was being fooled by
".." terms in the path strings.  This just adds a normalize step to
the path comparison and unmasks the missing warnings.

Andy

[PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Andy Ross]

In toolchain edge cases it's possible for the RPATH of a library to be
set to something like "/usr/lib/../lib".  This should be detected as
"/usr/lib" and generate a warning.  Also clarify the warning text to
indicate potential link-time pollution from the host libraries.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
 meta/classes/insane.bbclass | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 556a176..ade0616 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -161,6 +161,17 @@ def package_qa_check_rpath(file,name, d, elf, messages):
             if dir in line:
                 messages.append("package %s contains bad RPATH %s in file %s" % (name, line, file))
 
+def rpath_norm(s):
+    import re
+    s = re.sub('[^/]+/\.\.(/|$)', '/', s) # snip ".." components
+    s = re.sub('/\.(/|$)', '/', s)        # snip "." components
+    s = re.sub('/+', '/', s)              # snip repeated slashes
+    s = re.sub('/$', '', s)               # snip trailing slash
+    return s
+
+def rpath_eq(a, b):
+    return rpath_norm(a) == rpath_norm(b)
+
 QAPATHTEST[useless-rpaths] = "package_qa_check_useless_rpaths"
 def package_qa_check_useless_rpaths(file, name, d, elf, messages):
     """
@@ -181,10 +192,13 @@ def package_qa_check_useless_rpaths(file, name, d, elf, messages):
     	m = rpath_re.match(line)
 	if m:
 	   rpath = m.group(1)
-	   if rpath == libdir or rpath == base_libdir:
-	      # The dynamic linker searches both these places anyway.  There is no point in
-	      # looking there again.
-	      messages.append("%s: %s contains probably-redundant RPATH %s" % (name, package_qa_clean_path(file, d), rpath))
+	   if rpath_eq(rpath, libdir) or rpath_eq(rpath, base_libdir):
+	      # The dynamic linker searches both these places anyway.
+	      # There is no point in looking there again.  And it may
+	      # be harmful: the RPATH may point to host directories
+	      # (e.g. /usr/lib) which will be searched at link time,
+	      # creating build issues.
+	      messages.append("%s: %s contains probably-redundant, possibly host-polluted RPATH %s" % (name, package_qa_clean_path(file, d), rpath))
 
 QAPATHTEST[dev-so] = "package_qa_check_dev"
 def package_qa_check_dev(path, name, d, elf, messages):
-- 
1.7.11.2

Re: [PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Chris Larson]

On Wed, Aug 15, 2012 at 3:46 PM, Andy Ross <andy.ross@windriver.com> wrote:
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 556a176..ade0616 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -161,6 +161,17 @@ def package_qa_check_rpath(file,name, d, elf, messages):
>              if dir in line:
>                  messages.append("package %s contains bad RPATH %s in file %s" % (name, line, file))
>
> +def rpath_norm(s):
> +    import re
> +    s = re.sub('[^/]+/\.\.(/|$)', '/', s) # snip ".." components
> +    s = re.sub('/\.(/|$)', '/', s)        # snip "." components
> +    s = re.sub('/+', '/', s)              # snip repeated slashes
> +    s = re.sub('/
> , '', s)               # snip trailing slash
> +    return s
> +
> +def rpath_eq(a, b):
> +    return rpath_norm(a) == rpath_norm(b)
> +

Please just use os.path.normpath() rather than reinventing the wheel here.
-- 
Christopher Larson

Re: [PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Andy Ross]

Chris Larson wrote:
> Please just use os.path.normpath() rather than reinventing the wheel here.

Learn something new every day.  Fixed.

Andy

[PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Andy Ross]

In toolchain edge cases it's possible for the RPATH of a library to be
set to something like "/usr/lib/../lib".  This should be detected as
"/usr/lib" and generate a warning.  Also clarify the warning text to
indicate potential link-time pollution from the host libraries.

Signed-off-by: Andy Ross <andy.ross@windriver.com>
---
 meta/classes/insane.bbclass | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 556a176..dfaa9dc 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -161,6 +161,10 @@ def package_qa_check_rpath(file,name, d, elf, messages):
             if dir in line:
                 messages.append("package %s contains bad RPATH %s in file %s" % (name, line, file))
 
+def rpath_eq(a, b):
+    import os.path
+    return os.path.normpath(a) == os.path.normpath(b)
+
 QAPATHTEST[useless-rpaths] = "package_qa_check_useless_rpaths"
 def package_qa_check_useless_rpaths(file, name, d, elf, messages):
     """
@@ -181,10 +185,13 @@ def package_qa_check_useless_rpaths(file, name, d, elf, messages):
     	m = rpath_re.match(line)
 	if m:
 	   rpath = m.group(1)
-	   if rpath == libdir or rpath == base_libdir:
-	      # The dynamic linker searches both these places anyway.  There is no point in
-	      # looking there again.
-	      messages.append("%s: %s contains probably-redundant RPATH %s" % (name, package_qa_clean_path(file, d), rpath))
+	   if rpath_eq(rpath, libdir) or rpath_eq(rpath, base_libdir):
+	      # The dynamic linker searches both these places anyway.
+	      # There is no point in looking there again.  And it may
+	      # be harmful: the RPATH may point to host directories
+	      # (e.g. /usr/lib) which will be searched at link time,
+	      # creating build issues.
+	      messages.append("%s: %s contains probably-redundant, possibly host-polluted RPATH %s" % (name, package_qa_clean_path(file, d), rpath))
 
 QAPATHTEST[dev-so] = "package_qa_check_dev"
 def package_qa_check_dev(path, name, d, elf, messages):
-- 
1.7.11.2

Re: [PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Phil Blundell]

On Wed, 2012-08-15 at 15:46 -0700, Andy Ross wrote:
> In toolchain edge cases it's possible for the RPATH of a library to be
> set to something like "/usr/lib/../lib".  This should be detected as
> "/usr/lib" and generate a warning.  Also clarify the warning text to
> indicate potential link-time pollution from the host libraries.

If these RPATHs are causing host pollution then that sounds like there
is another bug elsewhere.  They ought to be resolved relative to the
sysroot during link edit.

It's also tempting to say that we should, unconditionally, normalise any
RPATHs which contain .. or . components before insane.bbclass runs.
Having extra, redundant, components in the pathname will just cause
extra work for ld.so (and hence run-time slowness) and bring no benefit.
Doing that would mean that the existing check would catch things like
the case you mentioned automatically.

p.

Re: [PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Andy Ross]

On 08/16/2012 01:39 AM, Phil Blundell wrote:
> If these RPATHs are causing host pollution then that sounds like there
> is another bug elsewhere.  They ought to be resolved relative to the
> sysroot during link edit.

Indeed, this is turning out to be a deeper issue than I wanted it to
be.  What seems to be is happening is this: an RPATH in the ELF header
is interpreted relative to sysroot normally.  But when linking, a
-rpath argument to the ld is interpreted relative to the *host*
filesystem even when there is a --sysroot argument.  See the quick
test script below.

As it happens, both of those are ultimately produced by libtool, and
it's only the second one that is fatal.  The RPATH itself is probably
still a warning condition, but it's not a build breaker.  But neither
is needed, they are happening in the case I'm looking at only because
libtool (I think) is confused by the "/../" syntax in the link path
into trying to add an RPATH where one isn't needed.

The specific case I'm looking at (with our internal tree, I'm working
on reproducing vs. poky right now) is with a pulseaudio build, where
an x86-64 build on an x86_64 host hits a RPATH in libgdk-x11-2.0 that
pulls in the host libXranr and fails due to version skew.  I was just
pointed at a discussion from last week on owl_video which looks all
but identical.

At least for the moment I'm going to try to track down the libtool
issue (maybe sanify the path before it sees if if possible) instead of
trying to fix a linker bug.

Andy

Reproducer for the underlying linker issue:

#!/bin/sh
set -ex

SYSROOT=/home/andy/oe/poky/build/tmp/sysroots/qemux86-64
CC=/home/andy/oe/poky/build/tmp/sysroots/x86_64-linux/usr/bin/x86_64-poky-linux/x86_64-poky-linux-gcc

# A library:
echo 'int foo(){return 0;}' > foo.c
$CC -shared -fPIC -o libfoo.so foo.c

# Another library that references the first via RPATH=`pwd`
echo 'int foo(); int bar(){return foo();}' > bar.c
$CC -shared -fPIC -o libbar.so bar.c -L. -lfoo -Wl,-rpath -Wl,`pwd`

# A program that uses the second library:
echo 'int bar(); int main(){return bar();}' > main.c

# Works (incorrectly!) because the "-rpath `pwd" argument here is
# *not* interpreted relative to sysroot, so the linker sees
# ./libfoo.so as a potential library.
$CC --sysroot=$SYSROOT -L$SYSROOT/usr/lib64 -Wl,-rpath -Wl,`pwd` -o main2 main.c libbar.so

echo THAT LINK SHOULD HAVE FAILED

# Fails (correctly) because nothing on the link line tells libbar.so
# how to find libfoo.so, nor does libfoo.so exist in the
# sysroot-relative RPATH.
$CC --sysroot=$SYSROOT -o main main.c libbar.so

Re: [PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Richard Purdie]

On Thu, 2012-08-16 at 10:43 -0700, Andy Ross wrote:
> On 08/16/2012 01:39 AM, Phil Blundell wrote:
> > If these RPATHs are causing host pollution then that sounds like there
> > is another bug elsewhere.  They ought to be resolved relative to the
> > sysroot during link edit.
> 
> Indeed, this is turning out to be a deeper issue than I wanted it to
> be.  What seems to be is happening is this: an RPATH in the ELF header
> is interpreted relative to sysroot normally.  But when linking, a
> -rpath argument to the ld is interpreted relative to the *host*
> filesystem even when there is a --sysroot argument.  See the quick
> test script below.
> 
> As it happens, both of those are ultimately produced by libtool, and
> it's only the second one that is fatal.  The RPATH itself is probably
> still a warning condition, but it's not a build breaker.  But neither
> is needed, they are happening in the case I'm looking at only because
> libtool (I think) is confused by the "/../" syntax in the link path
> into trying to add an RPATH where one isn't needed.
> 
> The specific case I'm looking at (with our internal tree, I'm working
> on reproducing vs. poky right now) is with a pulseaudio build, where
> an x86-64 build on an x86_64 host hits a RPATH in libgdk-x11-2.0 that
> pulls in the host libXranr and fails due to version skew.  I was just
> pointed at a discussion from last week on owl_video which looks all
> but identical.
> 
> At least for the moment I'm going to try to track down the libtool
> issue (maybe sanify the path before it sees if if possible) instead of
> trying to fix a linker bug.

I suspect you need to look somewhere around:

http://git.yoctoproject.org/cgit.cgi/poky/tree/meta/recipes-devtools/libtool/libtool/fix-rpath.patch

and normalise the rpaths being used by libtool. I think it has some kind
of path normalisation function somewhere...

Cheers,

Richard

Re: [PATCH] insane.bbclass: Fix RPATH warning in the face of funny path strings

[Andy Ross]

On 08/17/2012 03:28 AM, Richard Purdie wrote:
> I suspect you need to look somewhere around:
>
> http://git.yoctoproject.org/cgit.cgi/poky/tree/meta/recipes-devtools/libtool/libtool/fix-rpath.patch

Indeed.  Found that yesterday and am currently testing a fix.
Should be inbound within the hour.

Andy