| Message ID | 1489357076-31990-2-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
Thomas, All, On 2017-03-12 23:17 +0100, Thomas Petazzoni spake thusly: > As shown recently by the firejail example, it is easy to miss that a > package builds and installs binaries without actually cross-compiling > them: they are built for the host architecture instead of the target > architecture. > > This commit adds a small helper script, check-bin-arch, called as a > GLOBAL_INSTRUMENTATION_HOOKS at the end of the target installation of > each package, to verify that the files installed by this package have > been built for the correct architecture. > > Being called as a GLOBAL_INSTRUMENTATION_HOOKS allows the build to error > out right after the installation of the faulty package, and therefore > get autobuilder error detection properly assigned to this specific > package. > > Example output with the firejail package enabled, when building for an > ARM target: > > ERROR: ./usr/lib/firejail/libconnect.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM Of course, this no loner matches the new format in this v4. ;-) > ERROR: ./usr/bin/firejail (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/lib/firejail/libtrace.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/lib/firejail/libtracelog.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/lib/firejail/ftee (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/lib/firejail/faudit (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/bin/firemon (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/bin/firecfg (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > package/pkg-generic.mk:306: recipe for target '/home/thomas/projets/buildroot/output/build/firejail-0.9.44.8/.stamp_target_installed' failed > make[1]: *** [/home/thomas/projets/buildroot/output/build/firejail-0.9.44.8/.stamp_target_installed] Error 1 > > Many thanks to Yann E. Morin and Arnout Vandecappelle for their reviews > and suggestions. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Except for the nit in the commit log: Acked-by: "Yann E. MORIN" <yann.morin.1998@free.fr> Of course, there are a few things I'd have done slightly differently. But I did not do it, you did, so that's fine! ;-) Let's see what autobuilders have to say about this, now! Muhahaha! :-] Regards, Yann E. MORIN. > --- > Changes since v3: > - Pass TARGET_READELF instead of TARGET_CROSS, suggested by Arnout > - Pass BR2_READELF_ARCH_NAME to the script, instead of parsing the BR > config file. Suggested by Yann. > - Use LC_ALL=C when calling readelf, to avoid surprises. Suggested by > Yann. > - Reword the error message, according to Yann suggestion. > > Changes since v2: > - Moved as a per-package check, so that we detect the faulty package > earlier, and get correct autobuilder notifications. > - Use TARGET_DIR from the environment and use BR2_CONFIG to retrieve > the value of BR2_READELF_ARCH_NAME. > - Skip firmware files in /lib/firmware and /usr/lib/firmware. > - Simply ignore files for which readelf returned an empty Machine > field, as a way to quickly detect non-ELF files. > > Changes since v1: > - Following Yann E. Morin's comment, restrict the check to bin, lib, > sbin, usr/bin, usr/lib and usr/sbin, in order to avoid matching > firmware files that could use the ELF format but be targeted for > other architectures. > --- > package/pkg-generic.mk | 11 +++++++++++ > support/scripts/check-bin-arch | 39 +++++++++++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+) > create mode 100755 support/scripts/check-bin-arch > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index e8a8021..73e1bc2 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -87,6 +87,17 @@ define step_pkg_size > endef > GLOBAL_INSTRUMENTATION_HOOKS += step_pkg_size > > +# Relies on step_pkg_size, so must be after > +define check_bin_arch > + $(if $(filter end-install-target,$(1)-$(2)),\ > + support/scripts/check-bin-arch $(3) \ > + $(BUILD_DIR)/packages-file-list.txt \ > + $(TARGET_READELF) \ > + $(BR2_READELF_ARCH_NAME)) > +endef > + > +GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch > + > # This hook checks that host packages that need libraries that we build > # have a proper DT_RPATH or DT_RUNPATH tag > define check_host_rpath > diff --git a/support/scripts/check-bin-arch b/support/scripts/check-bin-arch > new file mode 100755 > index 0000000..63d941f > --- /dev/null > +++ b/support/scripts/check-bin-arch > @@ -0,0 +1,39 @@ > +#!/bin/bash > + > +PACKAGE=$1 > +PACKAGE_FILE_LIST=$2 > +TARGET_READELF=$3 > +READELF_ARCH_NAME=$4 > + > +exitcode=0 > + > +PKG_FILES=$(sed -r -e "/^${PACKAGE},(.+)$/!d; s//\1/;" ${PACKAGE_FILE_LIST}) > + > +for f in ${PKG_FILES} ; do > + # Skip firmware files, they could be ELF files for other > + # architectures > + if [[ "${f}" =~ ^\./(usr/)?lib/firmware/.* ]]; then > + continue > + fi > + > + # Get architecture using readelf > + arch=$(LC_ALL=C ${TARGET_READELF} -h "${TARGET_DIR}/${f}" 2>&1 | \ > + sed -r -e '/^ Machine: +(.+)/!d; s//\1/;') > + > + # If no architecture found, assume it was not an ELF file > + if test "${arch}" = "" ; then > + continue > + fi > + > + # Architecture is correct > + if test "${arch}" = "${READELF_ARCH_NAME}" ; then > + continue > + fi > + > + printf 'ERROR: architecture for %s (from %s) is %s, should be %s\n' \ > + "${f}" "${PACKAGE}" "${arch}" "${READELF_ARCH_NAME}" > + > + exitcode=1 > +done > + > +exit ${exitcode} > -- > 2.7.4 >
On 12-03-17 23:17, Thomas Petazzoni wrote: > As shown recently by the firejail example, it is easy to miss that a > package builds and installs binaries without actually cross-compiling > them: they are built for the host architecture instead of the target > architecture. > > This commit adds a small helper script, check-bin-arch, called as a > GLOBAL_INSTRUMENTATION_HOOKS at the end of the target installation of > each package, to verify that the files installed by this package have > been built for the correct architecture. > > Being called as a GLOBAL_INSTRUMENTATION_HOOKS allows the build to error > out right after the installation of the faulty package, and therefore > get autobuilder error detection properly assigned to this specific > package. > > Example output with the firejail package enabled, when building for an > ARM target: > > ERROR: ./usr/lib/firejail/libconnect.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/bin/firejail (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/lib/firejail/libtrace.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/lib/firejail/libtracelog.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/lib/firejail/ftee (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/lib/firejail/faudit (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/bin/firemon (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > ERROR: ./usr/bin/firecfg (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM > package/pkg-generic.mk:306: recipe for target '/home/thomas/projets/buildroot/output/build/firejail-0.9.44.8/.stamp_target_installed' failed > make[1]: *** [/home/thomas/projets/buildroot/output/build/firejail-0.9.44.8/.stamp_target_installed] Error 1 > > Many thanks to Yann E. Morin and Arnout Vandecappelle for their reviews > and suggestions. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> I almost gave my Ack, but there's still an issue... I also have a bunch of nits so the big issue is marked with [!!]. > --- [snip] > +# Relies on step_pkg_size, so must be after > +define check_bin_arch > + $(if $(filter end-install-target,$(1)-$(2)),\ > + support/scripts/check-bin-arch $(3) \ > + $(BUILD_DIR)/packages-file-list.txt \ > + $(TARGET_READELF) \ > + $(BR2_READELF_ARCH_NAME)) Yann's coding style, which is followed in most of our shell scripts, is to give arguments a flag if they are not something like a list of files. That makes it a lot more transparent what is happening. So in this case, you could have e.g. -p <package> -l <packages-file-list> -r <readelf> -a <arch>. Also, we usually do "$(call qstrip,$(BR2_READELF_ARCH_NAME))" to be sure the "" are there. Though honestly there is not so much reason to do that - the only valid reason is for variables that are overridden on the command line, but for this particular one we really don't need to support commandline override :-). > +endef > + > +GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch > + > # This hook checks that host packages that need libraries that we build > # have a proper DT_RPATH or DT_RUNPATH tag > define check_host_rpath > diff --git a/support/scripts/check-bin-arch b/support/scripts/check-bin-arch > new file mode 100755 > index 0000000..63d941f > --- /dev/null > +++ b/support/scripts/check-bin-arch > @@ -0,0 +1,39 @@ > +#!/bin/bash > + > +PACKAGE=$1 > +PACKAGE_FILE_LIST=$2 > +TARGET_READELF=$3 > +READELF_ARCH_NAME=$4 > + > +exitcode=0 > + > +PKG_FILES=$(sed -r -e "/^${PACKAGE},(.+)$/!d; s//\1/;" ${PACKAGE_FILE_LIST}) Fighting Yann on this one: I really find the "/.../s//\1/p" construct easier to parse than "/.../!d; s//\1/". Yes, found a bikeshed! > + > +for f in ${PKG_FILES} ; do > + # Skip firmware files, they could be ELF files for other > + # architectures > + if [[ "${f}" =~ ^\./(usr/)?lib/firmware/.* ]]; then > + continue > + fi > + > + # Get architecture using readelf > + arch=$(LC_ALL=C ${TARGET_READELF} -h "${TARGET_DIR}/${f}" 2>&1 | \ > + sed -r -e '/^ Machine: +(.+)/!d; s//\1/;') [!!] Since we now do this before the finalize step, TARGET_DIR is still full of .a files. These contain several ELF files, so readelf -h will print several ELF headers, so this will not match. We can safely assume that all ELF files in a .a are going to be for the same target, so just pipe into head -1. I tested that, so with that fixed, you can add my Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > + > + # If no architecture found, assume it was not an ELF file > + if test "${arch}" = "" ; then > + continue > + fi > + > + # Architecture is correct > + if test "${arch}" = "${READELF_ARCH_NAME}" ; then > + continue > + fi > + > + printf 'ERROR: architecture for %s (from %s) is %s, should be %s\n' \ ^^^^^^^^^ Since this is now called per-package, it isn't necessary any more to say from which package it comes. Regards, Arnout > + "${f}" "${PACKAGE}" "${arch}" "${READELF_ARCH_NAME}" > + > + exitcode=1 > +done > + > +exit ${exitcode} >
Arnout, All, On 2017-03-13 23:19 +0100, Arnout Vandecappelle spake thusly: > On 12-03-17 23:17, Thomas Petazzoni wrote: > > As shown recently by the firejail example, it is easy to miss that a > > package builds and installs binaries without actually cross-compiling > > them: they are built for the host architecture instead of the target > > architecture. > > > > This commit adds a small helper script, check-bin-arch, called as a > > GLOBAL_INSTRUMENTATION_HOOKS at the end of the target installation of > > each package, to verify that the files installed by this package have > > been built for the correct architecture. [--SNIP--] > > +PKG_FILES=$(sed -r -e "/^${PACKAGE},(.+)$/!d; s//\1/;" ${PACKAGE_FILE_LIST}) > > Fighting Yann on this one: I really find the "/.../s//\1/p" construct easier to > parse than "/.../!d; s//\1/". Yes, found a bikeshed! Yes, a bikeshed! :-) > > +for f in ${PKG_FILES} ; do > > + # Skip firmware files, they could be ELF files for other > > + # architectures > > + if [[ "${f}" =~ ^\./(usr/)?lib/firmware/.* ]]; then > > + continue > > + fi > > + > > + # Get architecture using readelf > > + arch=$(LC_ALL=C ${TARGET_READELF} -h "${TARGET_DIR}/${f}" 2>&1 | \ > > + sed -r -e '/^ Machine: +(.+)/!d; s//\1/;') > > [!!] Since we now do this before the finalize step, TARGET_DIR is still full of > .a files. These contain several ELF files, so readelf -h will print several ELF > headers, so this will not match. We can safely assume that all ELF files in a .a > are going to be for the same target, so just pipe into head -1. Or if you are concerned about a .a with multiple architectures (which would be highly awfull and questionable), you could sort the output with 'sort -u' so that: - if there is noly the correct arch, it will match, - if there is only one incorrect arch, it won't match - if there are more than one arch, at least one is incorrect and it won't match either. > I tested that, so with that fixed, you can add my > Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> > > > > + > > + # If no architecture found, assume it was not an ELF file > > + if test "${arch}" = "" ; then > > + continue > > + fi > > + > > + # Architecture is correct > > + if test "${arch}" = "${READELF_ARCH_NAME}" ; then > > + continue > > + fi > > + > > + printf 'ERROR: architecture for %s (from %s) is %s, should be %s\n' \ > ^^^^^^^^^ > Since this is now called per-package, it isn't necessary any more to say from > which package it comes. Indeed. Regards, Yann E. MORIN. > Regards, > Arnout > > > + "${f}" "${PACKAGE}" "${arch}" "${READELF_ARCH_NAME}" > > + > > + exitcode=1 > > +done > > + > > +exit ${exitcode} > > > > -- > Arnout Vandecappelle arnout at mind be > Senior Embedded Software Architect +32-16-286500 > Essensium/Mind http://www.mind.be > G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven > LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle > GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index e8a8021..73e1bc2 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -87,6 +87,17 @@ define step_pkg_size endef GLOBAL_INSTRUMENTATION_HOOKS += step_pkg_size +# Relies on step_pkg_size, so must be after +define check_bin_arch + $(if $(filter end-install-target,$(1)-$(2)),\ + support/scripts/check-bin-arch $(3) \ + $(BUILD_DIR)/packages-file-list.txt \ + $(TARGET_READELF) \ + $(BR2_READELF_ARCH_NAME)) +endef + +GLOBAL_INSTRUMENTATION_HOOKS += check_bin_arch + # This hook checks that host packages that need libraries that we build # have a proper DT_RPATH or DT_RUNPATH tag define check_host_rpath diff --git a/support/scripts/check-bin-arch b/support/scripts/check-bin-arch new file mode 100755 index 0000000..63d941f --- /dev/null +++ b/support/scripts/check-bin-arch @@ -0,0 +1,39 @@ +#!/bin/bash + +PACKAGE=$1 +PACKAGE_FILE_LIST=$2 +TARGET_READELF=$3 +READELF_ARCH_NAME=$4 + +exitcode=0 + +PKG_FILES=$(sed -r -e "/^${PACKAGE},(.+)$/!d; s//\1/;" ${PACKAGE_FILE_LIST}) + +for f in ${PKG_FILES} ; do + # Skip firmware files, they could be ELF files for other + # architectures + if [[ "${f}" =~ ^\./(usr/)?lib/firmware/.* ]]; then + continue + fi + + # Get architecture using readelf + arch=$(LC_ALL=C ${TARGET_READELF} -h "${TARGET_DIR}/${f}" 2>&1 | \ + sed -r -e '/^ Machine: +(.+)/!d; s//\1/;') + + # If no architecture found, assume it was not an ELF file + if test "${arch}" = "" ; then + continue + fi + + # Architecture is correct + if test "${arch}" = "${READELF_ARCH_NAME}" ; then + continue + fi + + printf 'ERROR: architecture for %s (from %s) is %s, should be %s\n' \ + "${f}" "${PACKAGE}" "${arch}" "${READELF_ARCH_NAME}" + + exitcode=1 +done + +exit ${exitcode}
As shown recently by the firejail example, it is easy to miss that a package builds and installs binaries without actually cross-compiling them: they are built for the host architecture instead of the target architecture. This commit adds a small helper script, check-bin-arch, called as a GLOBAL_INSTRUMENTATION_HOOKS at the end of the target installation of each package, to verify that the files installed by this package have been built for the correct architecture. Being called as a GLOBAL_INSTRUMENTATION_HOOKS allows the build to error out right after the installation of the faulty package, and therefore get autobuilder error detection properly assigned to this specific package. Example output with the firejail package enabled, when building for an ARM target: ERROR: ./usr/lib/firejail/libconnect.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM ERROR: ./usr/bin/firejail (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM ERROR: ./usr/lib/firejail/libtrace.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM ERROR: ./usr/lib/firejail/libtracelog.so (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM ERROR: ./usr/lib/firejail/ftee (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM ERROR: ./usr/lib/firejail/faudit (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM ERROR: ./usr/bin/firemon (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM ERROR: ./usr/bin/firecfg (from firejail) architecture is Advanced Micro Devices X86-64, should be ARM package/pkg-generic.mk:306: recipe for target '/home/thomas/projets/buildroot/output/build/firejail-0.9.44.8/.stamp_target_installed' failed make[1]: *** [/home/thomas/projets/buildroot/output/build/firejail-0.9.44.8/.stamp_target_installed] Error 1 Many thanks to Yann E. Morin and Arnout Vandecappelle for their reviews and suggestions. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- Changes since v3: - Pass TARGET_READELF instead of TARGET_CROSS, suggested by Arnout - Pass BR2_READELF_ARCH_NAME to the script, instead of parsing the BR config file. Suggested by Yann. - Use LC_ALL=C when calling readelf, to avoid surprises. Suggested by Yann. - Reword the error message, according to Yann suggestion. Changes since v2: - Moved as a per-package check, so that we detect the faulty package earlier, and get correct autobuilder notifications. - Use TARGET_DIR from the environment and use BR2_CONFIG to retrieve the value of BR2_READELF_ARCH_NAME. - Skip firmware files in /lib/firmware and /usr/lib/firmware. - Simply ignore files for which readelf returned an empty Machine field, as a way to quickly detect non-ELF files. Changes since v1: - Following Yann E. Morin's comment, restrict the check to bin, lib, sbin, usr/bin, usr/lib and usr/sbin, in order to avoid matching firmware files that could use the ELF format but be targeted for other architectures. --- package/pkg-generic.mk | 11 +++++++++++ support/scripts/check-bin-arch | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100755 support/scripts/check-bin-arch