diff mbox series

[1/1] Makefile: strip even non-writable files

Message ID 20250602110006.3257934-1-gilles.bardoux@sagemcom.com
State Changes Requested
Headers show
Series [1/1] Makefile: strip even non-writable files | expand

Commit Message

Gilles Bardoux June 2, 2025, 11 a.m. UTC 
Currently, files without write permission are not stripped.
Since calling a find exec script for all the elf files would be
slower than doing it only for the files without write permission,
the STRIP_FIND_CMD has been split into two finds:
 - one for the files with write permission for the current user,
   without an exec script
 - one for the files witout write permission for the current user
   calling the exec script
The find exec script (non-writable-file-cmd-wrapper) has been made
as generic as possible. It does a chmod u+w before the command (here
a strip) and restores the original file permissions afterwards.
The find command for special files (libpthread and ld-*.so) was not
split, because it concerns only a few files. So the find exec script
is called in all cases for these files.

Signed-off-by: Gilles Bardoux <gilles.bardoux@sagemcom.com>
---
 Makefile                                      | 20 ++++++++--
 support/scripts/non-writable-file-cmd-wrapper | 37 +++++++++++++++++++
 2 files changed, 53 insertions(+), 4 deletions(-)
 create mode 100755 support/scripts/non-writable-file-cmd-wrapper

Comments

Arnout Vandecappelle June 4, 2025, 7:24 p.m. UTC | #1 
Hi Gilles,

On 02/06/2025 13:00, Gilles Bardoux via buildroot wrote:
> Currently, files without write permission are not stripped.

  Good idea to fix that!

> Since calling a find exec script for all the elf files would be
> slower than doing it only for the files without write permission,

  With -exec yes, but if you use the original -print0 | xargs -0 then the helper 
script just gets called once (or a few times) with many arguments. In the script 
you can do

for file; do ...; done

which is almost as efficient as find -exec.

  It _is_ indeed a bit more efficient to call the bare strip with many arguments 
rather than re-starting it all the time, but I doubt that you can measure a 
significant difference. It's a small executable with few shared libs so 
launching it is not so expensive.

> the STRIP_FIND_CMD has been split into two finds:
>   - one for the files with write permission for the current user,
>     without an exec script
>   - one for the files witout write permission for the current user
>     calling the exec script
> The find exec script (non-writable-file-cmd-wrapper) has been made
> as generic as possible. It does a chmod u+w before the command (here
> a strip) and restores the original file permissions afterwards.
> The find command for special files (libpthread and ld-*.so) was not
> split, because it concerns only a few files. So the find exec script
> is called in all cases for these files.

  This is starting to sound super complicated...

  I think it would be better to move all the logic to distinguish between the 
cases where it needs to be made writable and the cases where it needs to use the 
special debug strip into the helper script.

  In fact, even the find command itself could move in there - though that is 
less of a factor I think, because you'd need to export quite a few variables to 
that script.


> Signed-off-by: Gilles Bardoux <gilles.bardoux@sagemcom.com>
> ---
>   Makefile                                      | 20 ++++++++--
>   support/scripts/non-writable-file-cmd-wrapper | 37 +++++++++++++++++++
>   2 files changed, 53 insertions(+), 4 deletions(-)
>   create mode 100755 support/scripts/non-writable-file-cmd-wrapper
> 
> diff --git a/Makefile b/Makefile
> index f90b834ed2..cf2b517938 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -637,17 +637,28 @@ STRIP_FIND_COMMON_CMD = \
>   #   applications and libraries. Normally kernel modules are already excluded
>   #   by the executable permission check, so the explicit exclusion is only
>   #   done for kernel modules with incorrect permissions.
> -STRIP_FIND_CMD = \
> +STRIP_FIND_WRITABLE_CMD = \
>   	$(STRIP_FIND_COMMON_CMD) \
>   	-type f \( -perm /111 -o -name '*.so*' \) \
>   	-not \( $(call findfileclauses,libpthread*.so* ld-*.so* *.ko) \) \
> +	-writable \
>   	-print0
>   
> +# Stripping for non writables files
> +STRIP_FIND_NOT_WRITABLE_CMD = \
> +	WRAPPED_CMD="$(STRIPCMD)" \
> +	$(STRIP_FIND_COMMON_CMD) \
> +	-type f \( -perm /111 -o -name '*.so*' \) \
> +	-not \( $(call findfileclauses,libpthread*.so* ld-*.so* *.ko) \) \
> +	-not -writable \
> +	-exec $(TOPDIR)/support/scripts/non-writable-file-cmd-wrapper {} \;
> +
>   # Special stripping (only debugging symbols) for libpthread and ld-*.so.
>   STRIP_FIND_SPECIAL_LIBS_CMD = \
> +	WRAPPED_CMD="$(STRIPCMD) $(STRIP_STRIP_DEBUG)" \
>   	$(STRIP_FIND_COMMON_CMD) \
>   	\( -name 'ld-*.so*' -o -name 'libpthread*.so*' \) \
> -	-print0
> +	-exec $(TOPDIR)/support/scripts/non-writable-file-cmd-wrapper {} \;
>   
>   # Generate locale data.
>   ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),y)
> @@ -756,8 +767,9 @@ endif
>   ifneq ($(BR2_ENABLE_DEBUG):$(BR2_STRIP_strip),y:)
>   	rm -rf $(TARGET_DIR)/lib/debug $(TARGET_DIR)/usr/lib/debug
>   endif
> -	$(STRIP_FIND_CMD) | xargs -0 $(STRIPCMD) 2>/dev/null || true
> -	$(STRIP_FIND_SPECIAL_LIBS_CMD) | xargs -0 -r $(STRIPCMD) $(STRIP_STRIP_DEBUG) 2>/dev/null || true
> +	$(STRIP_FIND_WRITABLE_CMD) | xargs -0 $(STRIPCMD) 2>/dev/null || true
> +	$(STRIP_FIND_NOT_WRITABLE_CMD) 2>/dev/null || true
> +	$(STRIP_FIND_SPECIAL_LIBS_CMD) 2>/dev/null || true

  So here we would have:

         find $(TARGET_DIR) \
         $(if $(call qstrip,$(BR2_STRIP_EXCLUDE_DIRS)), \
                 \( $(call finddirclauses,$(TARGET_DIR),$(call 
qstrip,$(BR2_STRIP_EXCLUDE_DIRS))) \) \
                 -prune -o \
         ) \
         $(if $(call qstrip,$(BR2_STRIP_EXCLUDE_FILES)), \
                 -not \( $(call findfileclauses,$(call 
qstrip,$(BR2_STRIP_EXCLUDE_FILES))) \) ) \
	-type f \( -perm /111 -o -name '*.so*' \) \
	-print0 \
	| STRIPCMD="$(STRIPCMD)" STRIP_STRIP_DEBUG="$(STRIP_STRIP_DEBUG)" \
	xargs -0 support/scripts/strip-helper


  Note that the different STRIP_FIND variables can all be removed.
  Note that no || true is needed because the strip-helper doesn't return errors.

>   
>   	test -f $(TARGET_DIR)/etc/ld.so.conf && \
>   		{ echo "ERROR: we shouldn't have a /etc/ld.so.conf file"; exit 1; } || true
> diff --git a/support/scripts/non-writable-file-cmd-wrapper b/support/scripts/non-writable-file-cmd-wrapper
> new file mode 100755
> index 0000000000..efbbeb6e7c
> --- /dev/null
> +++ b/support/scripts/non-writable-file-cmd-wrapper
> @@ -0,0 +1,37 @@
> +#!/usr/bin/env bash
> +
> +usage() {
> +  cat <<EOF >&2
> +Usage:  ${0} FILE
> +
> +Description:
> +
> +    This script runs WRAPPED_CMD with the file given in argument, adding the write permission temporarily during this action.
> +
> +Arguments:
> +
> +    FILE         file path to give to the command
> +
> +Environment:
> +
> +    WRAPPED_CMD  core command to run in between the chmods
> +
> +Returns:         0
> +
> +EOF
> +}

  This usage() function isn't called. In the support scripts we typically put 
the usage in comments because they're anyway only supposed to be called from the 
Makefile.

> +
> +helper() {

  We typically call it main().


> +    local file="${1}"
> +
> +    # make files writable if necessary
> +    changed="$(chmod -c u+w "${file}")"
> +
> +    # action on the file
> +    ${WRAPPED_CMD} "${file}"
> +
> +    # restore the original permission
> +    test "${changed}" != "" && chmod u-w "${file}"

  I think we normally use test -z "${changed}" to test for empty string.

  Regards,
  Arnout

> +}
> +helper "${@}"
> +exit 0
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index f90b834ed2..cf2b517938 100644
--- a/Makefile
+++ b/Makefile
@@ -637,17 +637,28 @@  STRIP_FIND_COMMON_CMD = \
 #   applications and libraries. Normally kernel modules are already excluded
 #   by the executable permission check, so the explicit exclusion is only
 #   done for kernel modules with incorrect permissions.
-STRIP_FIND_CMD = \
+STRIP_FIND_WRITABLE_CMD = \
 	$(STRIP_FIND_COMMON_CMD) \
 	-type f \( -perm /111 -o -name '*.so*' \) \
 	-not \( $(call findfileclauses,libpthread*.so* ld-*.so* *.ko) \) \
+	-writable \
 	-print0
 
+# Stripping for non writables files
+STRIP_FIND_NOT_WRITABLE_CMD = \
+	WRAPPED_CMD="$(STRIPCMD)" \
+	$(STRIP_FIND_COMMON_CMD) \
+	-type f \( -perm /111 -o -name '*.so*' \) \
+	-not \( $(call findfileclauses,libpthread*.so* ld-*.so* *.ko) \) \
+	-not -writable \
+	-exec $(TOPDIR)/support/scripts/non-writable-file-cmd-wrapper {} \;
+
 # Special stripping (only debugging symbols) for libpthread and ld-*.so.
 STRIP_FIND_SPECIAL_LIBS_CMD = \
+	WRAPPED_CMD="$(STRIPCMD) $(STRIP_STRIP_DEBUG)" \
 	$(STRIP_FIND_COMMON_CMD) \
 	\( -name 'ld-*.so*' -o -name 'libpthread*.so*' \) \
-	-print0
+	-exec $(TOPDIR)/support/scripts/non-writable-file-cmd-wrapper {} \;
 
 # Generate locale data.
 ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),y)
@@ -756,8 +767,9 @@  endif
 ifneq ($(BR2_ENABLE_DEBUG):$(BR2_STRIP_strip),y:)
 	rm -rf $(TARGET_DIR)/lib/debug $(TARGET_DIR)/usr/lib/debug
 endif
-	$(STRIP_FIND_CMD) | xargs -0 $(STRIPCMD) 2>/dev/null || true
-	$(STRIP_FIND_SPECIAL_LIBS_CMD) | xargs -0 -r $(STRIPCMD) $(STRIP_STRIP_DEBUG) 2>/dev/null || true
+	$(STRIP_FIND_WRITABLE_CMD) | xargs -0 $(STRIPCMD) 2>/dev/null || true
+	$(STRIP_FIND_NOT_WRITABLE_CMD) 2>/dev/null || true
+	$(STRIP_FIND_SPECIAL_LIBS_CMD) 2>/dev/null || true
 
 	test -f $(TARGET_DIR)/etc/ld.so.conf && \
 		{ echo "ERROR: we shouldn't have a /etc/ld.so.conf file"; exit 1; } || true
diff --git a/support/scripts/non-writable-file-cmd-wrapper b/support/scripts/non-writable-file-cmd-wrapper
new file mode 100755
index 0000000000..efbbeb6e7c
--- /dev/null
+++ b/support/scripts/non-writable-file-cmd-wrapper
@@ -0,0 +1,37 @@ 
+#!/usr/bin/env bash
+
+usage() {
+  cat <<EOF >&2
+Usage:  ${0} FILE
+
+Description:
+
+    This script runs WRAPPED_CMD with the file given in argument, adding the write permission temporarily during this action.
+
+Arguments:
+
+    FILE         file path to give to the command
+
+Environment:
+
+    WRAPPED_CMD  core command to run in between the chmods
+
+Returns:         0
+
+EOF
+}
+
+helper() {
+    local file="${1}"
+
+    # make files writable if necessary
+    changed="$(chmod -c u+w "${file}")"
+
+    # action on the file
+    ${WRAPPED_CMD} "${file}"
+
+    # restore the original permission
+    test "${changed}" != "" && chmod u-w "${file}"
+}
+helper "${@}"
+exit 0