Patchwork [v2] insane: make GNU_HASH check slightly more robust (avoids false negatives with gold); add check for useless rpaths

login
register
mail settings
Submitter Phil Blundell
Date July 14, 2011, 9:02 a.m.
Message ID <1310634132.2378.36.camel@phil-desktop>
Download mbox | patch
Permalink /patch/7593/
State New, archived
Headers show

Comments

Phil Blundell - July 14, 2011, 9:02 a.m.
It isn't safe to make assumptions about the order of the entries in the dynamic section.  Fix the ldflags test to cope with the case where GNU_HASH comes before NEEDED and/or INIT.

Also, add a new warning for binaries which contain useless (but benign) rpath entries pointing to the default search locations.

Signed-off-by: Phil Blundell <philb@gnu.org>
---
v2: fix typo in first version

 meta/classes/insane.bbclass |   44 ++++++++++++++++++++++++++++++++----------
 1 files changed, 33 insertions(+), 11 deletions(-)
Richard Purdie - July 14, 2011, 2:07 p.m.
On Thu, 2011-07-14 at 10:02 +0100, Phil Blundell wrote:
> It isn't safe to make assumptions about the order of the entries in the dynamic section.  Fix the ldflags test to cope with the case where GNU_HASH comes before NEEDED and/or INIT.
> 
> Also, add a new warning for binaries which contain useless (but benign) rpath entries pointing to the default search locations.
> 
> Signed-off-by: Phil Blundell <philb@gnu.org>
> ---
> v2: fix typo in first version

Merged to master, thanks.

Richard
Koen Kooi - July 14, 2011, 7:26 p.m.
Op 14 jul 2011, om 11:02 heeft Phil Blundell het volgende geschreven:

> It isn't safe to make assumptions about the order of the entries in the dynamic section.  Fix the ldflags test to cope with the case where GNU_HASH comes before NEEDED and/or INIT.
> 
> Also, add a new warning for binaries which contain useless (but benign) rpath entries pointing to the default search locations.

The new rpath warning is nice, but a bit too terse:

NOTE: package tinylogin-1.4-r7: task do_rm_work: Succeeded
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
WARNING: QA Issue: dynamic section contains probably-redundant RPATH /lib
NOTE: package shadow-4.1.4.3-r2: task do_package: Succeeded

I suspect it's shadow that has the rpaths, but I'm not sure. It think changing

> messages.append("dynamic section contains probably-redundant RPATH %s" % rpath)

to something like

> messages.append("dynamic section contains probably-redundant RPATH %s in %s" % (rpath, path))

Would improve things.

regards,

Koen

> 
> Signed-off-by: Phil Blundell <philb@gnu.org>
> ---
> v2: fix typo in first version
> 
> meta/classes/insane.bbclass |   44 ++++++++++++++++++++++++++++++++----------
> 1 files changed, 33 insertions(+), 11 deletions(-)
> 
> diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
> index 8d5da00..c45f2cb 100644
> --- a/meta/classes/insane.bbclass
> +++ b/meta/classes/insane.bbclass
> @@ -92,7 +92,7 @@ def package_qa_get_machine_dict():
>        }
> 
> 
> -WARN_QA ?= "dev-so rpaths debug-deps dev-deps debug-files arch la2 pkgconfig desktop la ldflags perms"
> +WARN_QA ?= "dev-so rpaths debug-deps dev-deps debug-files arch la2 pkgconfig desktop la ldflags perms useless-rpaths"
> ERROR_QA ?= ""
> #ERROR_QA ?= "rpaths debug-deps dev-deps debug-files arch pkgconfig perms"
> 
> @@ -141,6 +141,31 @@ 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))
> 
> +QAPATHTEST[useless-rpaths] = "package_qa_check_useless_rpaths"
> +def package_qa_check_useless_rpaths(file,name, d, elf, messages):
> +    """
> +    Check for RPATHs that are useless but not dangerous
> +    """
> +    if not elf:
> +        return
> +
> +    objdump = bb.data.getVar('OBJDUMP', d, True)
> +    env_path = bb.data.getVar('PATH', d, True)
> +
> +    libdir = bb.data.getVar("libdir", d, True)
> +    base_libdir = bb.data.getVar("base_libdir", d, True)
> +
> +    import re
> +    rpath_re = re.compile("\s+RPATH\s+(.*)")
> +    for line in os.popen("LC_ALL=C PATH=%s %s -p '%s' 2> /dev/null" % (env_path, objdump, file), "r"):
> +    	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("dynamic section contains probably-redundant RPATH %s" % rpath)
> +
> QAPATHTEST[dev-so] = "package_qa_check_dev"
> def package_qa_check_dev(path, name, d, elf, messages):
>     """
> @@ -238,22 +263,19 @@ def package_qa_hash_style(path, name, d, elf, messages):
>     objdump = bb.data.getVar('OBJDUMP', d, True)
>     env_path = bb.data.getVar('PATH', d, True)
> 
> -    sane = True
> -    elf = False
> -    # A bit hacky. We do not know if path is an elf binary or not
> -    # we will search for 'NEEDED' or 'INIT' as this should be printed...
> -    # and come before the HASH section (guess!!!) and works on split out
> -    # debug symbols too
> +    sane = False
> +    has_syms = False
> +
> +    # If this binary has symbols, we expect it to have GNU_HASH too.
>     for line in os.popen("LC_ALL=C PATH=%s %s -p '%s' 2> /dev/null" % (env_path, objdump, path), "r"):
> -        if "NEEDED" in line or "INIT" in line:
> -            sane = False
> -            elf = True
> +        if "SYMTAB" in line:
> +            has_syms = True
>         if "GNU_HASH" in line:
>             sane = True
>         if "[mips32]" in line or "[mips64]" in line:
> 	    sane = True
> 
> -    if elf and not sane:
> +    if has_syms and not sane:
>         messages.append("No GNU_HASH in the elf binary: '%s'" % path)
> 
> 
> -- 
> 1.7.4.1
> 
> 
> 
> 
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.linuxtogo.org/cgi-bin/mailman/listinfo/openembedded-core

Patch

diff --git a/meta/classes/insane.bbclass b/meta/classes/insane.bbclass
index 8d5da00..c45f2cb 100644
--- a/meta/classes/insane.bbclass
+++ b/meta/classes/insane.bbclass
@@ -92,7 +92,7 @@  def package_qa_get_machine_dict():
        }
 
 
-WARN_QA ?= "dev-so rpaths debug-deps dev-deps debug-files arch la2 pkgconfig desktop la ldflags perms"
+WARN_QA ?= "dev-so rpaths debug-deps dev-deps debug-files arch la2 pkgconfig desktop la ldflags perms useless-rpaths"
 ERROR_QA ?= ""
 #ERROR_QA ?= "rpaths debug-deps dev-deps debug-files arch pkgconfig perms"
 
@@ -141,6 +141,31 @@  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))
 
+QAPATHTEST[useless-rpaths] = "package_qa_check_useless_rpaths"
+def package_qa_check_useless_rpaths(file,name, d, elf, messages):
+    """
+    Check for RPATHs that are useless but not dangerous
+    """
+    if not elf:
+        return
+
+    objdump = bb.data.getVar('OBJDUMP', d, True)
+    env_path = bb.data.getVar('PATH', d, True)
+
+    libdir = bb.data.getVar("libdir", d, True)
+    base_libdir = bb.data.getVar("base_libdir", d, True)
+
+    import re
+    rpath_re = re.compile("\s+RPATH\s+(.*)")
+    for line in os.popen("LC_ALL=C PATH=%s %s -p '%s' 2> /dev/null" % (env_path, objdump, file), "r"):
+    	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("dynamic section contains probably-redundant RPATH %s" % rpath)
+
 QAPATHTEST[dev-so] = "package_qa_check_dev"
 def package_qa_check_dev(path, name, d, elf, messages):
     """
@@ -238,22 +263,19 @@  def package_qa_hash_style(path, name, d, elf, messages):
     objdump = bb.data.getVar('OBJDUMP', d, True)
     env_path = bb.data.getVar('PATH', d, True)
 
-    sane = True
-    elf = False
-    # A bit hacky. We do not know if path is an elf binary or not
-    # we will search for 'NEEDED' or 'INIT' as this should be printed...
-    # and come before the HASH section (guess!!!) and works on split out
-    # debug symbols too
+    sane = False
+    has_syms = False
+
+    # If this binary has symbols, we expect it to have GNU_HASH too.
     for line in os.popen("LC_ALL=C PATH=%s %s -p '%s' 2> /dev/null" % (env_path, objdump, path), "r"):
-        if "NEEDED" in line or "INIT" in line:
-            sane = False
-            elf = True
+        if "SYMTAB" in line:
+            has_syms = True
         if "GNU_HASH" in line:
             sane = True
         if "[mips32]" in line or "[mips64]" in line:
 	    sane = True
 
-    if elf and not sane:
+    if has_syms and not sane:
         messages.append("No GNU_HASH in the elf binary: '%s'" % path)