[RFC/PATCH] Makefile: guarantee reproducible permissions

Currently, the permission mode on many target rootfs files depend on the
user's umask at the time of building, and at the time of cloning the
repo. This is caused by two things:

1) Some packages and BR itself create files and directories on the
target with cp/mkdir/etc which depend on the umask at the time of
building.

2) We use rsync -a to copy the skeleton and overlay, leaving permissions
on the target exactly as they were on the host. These permissions are
not tracked by Git and depend on the user's umask at the time of cloning
(assuming no mode changes).

To fix (1), change the Makefile's $(SHELL) to always call a wrapper
script first that sets the umask to a sane fixed value (022) and then
calls the real shell.

To fix (2), use the --chmod option on rsync calls so we don't depend on
the current mode of those files.

Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
---
 Makefile                         | 7 ++++---
 support/scripts/shell-wrapper.sh | 5 +++++
 2 files changed, 9 insertions(+), 3 deletions(-)
 create mode 100755 support/scripts/shell-wrapper.sh

On 03/11/14 13:42, Guido Martínez wrote:
> Currently, the permission mode on many target rootfs files depend on the
> user's umask at the time of building, and at the time of cloning the
> repo. This is caused by two things:
> 
> 1) Some packages and BR itself create files and directories on the
> target with cp/mkdir/etc which depend on the umask at the time of
> building.
> 
> 2) We use rsync -a to copy the skeleton and overlay, leaving permissions
> on the target exactly as they were on the host. These permissions are
> not tracked by Git and depend on the user's umask at the time of cloning
> (assuming no mode changes).
> 
> To fix (1), change the Makefile's $(SHELL) to always call a wrapper
> script first that sets the umask to a sane fixed value (022) and then
> calls the real shell.
> 
> To fix (2), use the --chmod option on rsync calls so we don't depend on
> the current mode of those files.
> 
> Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>

 I build-tested with and without the patch, compared the permissions and they're
identical.

 I was also a bit concerned about the effect on build time of the additional
fork, but the difference turns out to be smaller than the noise margin.


 However, you did miss a few rsync/cp instances that could be problematic.

- The external toolchain uses a combination of rsync and cp to copy the sysroot
to the target. Normally not an issue, unless when you download and extract the
external toolchain yourself and use BR2_TOOLCHAIN_EXTERNAL_PATH.

- The OVERRIDE_SRCDIR infrastructure in general may show the same problem if
stuff is then cp'ed into the target. However, that one we probably shouldn't
bother with too much. Still, adding a chmod to the rsync in the package
infrastructure is a possibility.

 I think these two should be fixed before accepting the patch.


 Also, there are probably a couple of packages that use cp to copy stuff from
the buildroot directory to the target. A quick search only turns up
matchbox-keyboard, but there may be more. So perhaps you could do an
allpackageyes build and see if anything turns up in the target directory that is
not world-readable. But even without that, the patch is acceptable after just
fixing the external toolchain and OVERRIDE_SRCDIR support.


 Regards,
 Arnout


> ---
>  Makefile                         | 7 ++++---
>  support/scripts/shell-wrapper.sh | 5 +++++
>  2 files changed, 9 insertions(+), 3 deletions(-)
>  create mode 100755 support/scripts/shell-wrapper.sh
> 
> diff --git a/Makefile b/Makefile
> index 907a0fc..0f7db1e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -203,7 +203,8 @@ else
>  endif
>  
>  # we want bash as shell
> -SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
> +SHELL := $(TOPDIR)/support/scripts/shell-wrapper.sh \
> +	 $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
>  	 else if [ -x /bin/bash ]; then echo /bin/bash; \
>  	 else echo sh; fi; fi)
>  
> @@ -476,7 +477,7 @@ RSYNC_VCS_EXCLUSIONS = \
>  $(BUILD_DIR)/.root:
>  	mkdir -p $(TARGET_DIR)
>  	rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
> -		--chmod=Du+w --exclude .empty --exclude '*~' \
> +		--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
>  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
>  	$(INSTALL) -m 0644 support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
>  	@ln -snf lib $(TARGET_DIR)/$(LIB_SYMLINK)
> @@ -612,7 +613,7 @@ endif
>  	@$(foreach d, $(call qstrip,$(BR2_ROOTFS_OVERLAY)), \
>  		$(call MESSAGE,"Copying overlay $(d)"); \
>  		rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
> -			--chmod=Du+w --exclude .empty --exclude '*~' \
> +			--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
>  			$(d)/ $(TARGET_DIR)$(sep))
>  
>  	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_BUILD_SCRIPT)), \
> diff --git a/support/scripts/shell-wrapper.sh b/support/scripts/shell-wrapper.sh
> new file mode 100755
> index 0000000..513b927
> --- /dev/null
> +++ b/support/scripts/shell-wrapper.sh
> @@ -0,0 +1,5 @@
> +#!/bin/sh
> +
> +umask 022
> +
> +exec "$@"
>

On 11/03/2014 07:23 PM, Arnout Vandecappelle wrote:
[..]
> 
>  Also, there are probably a couple of packages that use cp to copy stuff from
> the buildroot directory to the target. A quick search only turns up
> matchbox-keyboard, but there may be more. So perhaps you could do an
> allpackageyes build and see if anything turns up in the target directory that is
> not world-readable. But even without that, the patch is acceptable after just
> fixing the external toolchain and OVERRIDE_SRCDIR support.
> 

Does it make sense to add a few remarks about the rootfs permissions to
the manual as well? That way you won't have to answer people each time
they ask about this, and instead just point to the manual.

Hi Arnout,

first of all thanks for the review!

On Mon, Nov 03, 2014 at 11:23:53PM +0100, Arnout Vandecappelle wrote:
> On 03/11/14 13:42, Guido Martínez wrote:
> > Currently, the permission mode on many target rootfs files depend on the
> > user's umask at the time of building, and at the time of cloning the
> > repo. This is caused by two things:
> > 
> > 1) Some packages and BR itself create files and directories on the
> > target with cp/mkdir/etc which depend on the umask at the time of
> > building.
> > 
> > 2) We use rsync -a to copy the skeleton and overlay, leaving permissions
> > on the target exactly as they were on the host. These permissions are
> > not tracked by Git and depend on the user's umask at the time of cloning
> > (assuming no mode changes).
> > 
> > To fix (1), change the Makefile's $(SHELL) to always call a wrapper
> > script first that sets the umask to a sane fixed value (022) and then
> > calls the real shell.
> > 
> > To fix (2), use the --chmod option on rsync calls so we don't depend on
> > the current mode of those files.
> > 
> > Signed-off-by: Guido Martínez <guido@vanguardiasur.com.ar>
> 
>  I build-tested with and without the patch, compared the permissions and they're
> identical.
> 
>  I was also a bit concerned about the effect on build time of the additional
> fork, but the difference turns out to be smaller than the noise margin.
> 
> 
>  However, you did miss a few rsync/cp instances that could be problematic.
> 
> - The external toolchain uses a combination of rsync and cp to copy the sysroot
> to the target. Normally not an issue, unless when you download and extract the
> external toolchain yourself and use BR2_TOOLCHAIN_EXTERNAL_PATH.
Are you sure? I think rsync is used to copy the sysroot to the staging
dir (output/host). The target is then filled with various 'install -m's
for each library, so the previous set of permissions wouldn't matter.

> - The OVERRIDE_SRCDIR infrastructure in general may show the same problem if
> stuff is then cp'ed into the target. However, that one we probably shouldn't
> bother with too much. Still, adding a chmod to the rsync in the package
> infrastructure is a possibility.
Right! For archives, we currently do "chmod -R +rw". Maybe we should set
them both to u=rwX,go=rX ?

>  I think these two should be fixed before accepting the patch.
> 
> 
>  Also, there are probably a couple of packages that use cp to copy stuff from
> the buildroot directory to the target. A quick search only turns up
> matchbox-keyboard, but there may be more. So perhaps you could do an
> allpackageyes build and see if anything turns up in the target directory that is
> not world-readable. But even without that, the patch is acceptable after just
> fixing the external toolchain and OVERRIDE_SRCDIR support.
I'll take a deeper look at this.

Thanks!

> 
> 
>  Regards,
>  Arnout
> 
> 
> > ---
> >  Makefile                         | 7 ++++---
> >  support/scripts/shell-wrapper.sh | 5 +++++
> >  2 files changed, 9 insertions(+), 3 deletions(-)
> >  create mode 100755 support/scripts/shell-wrapper.sh
> > 
> > diff --git a/Makefile b/Makefile
> > index 907a0fc..0f7db1e 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -203,7 +203,8 @@ else
> >  endif
> >  
> >  # we want bash as shell
> > -SHELL := $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
> > +SHELL := $(TOPDIR)/support/scripts/shell-wrapper.sh \
> > +	 $(shell if [ -x "$$BASH" ]; then echo $$BASH; \
> >  	 else if [ -x /bin/bash ]; then echo /bin/bash; \
> >  	 else echo sh; fi; fi)
> >  
> > @@ -476,7 +477,7 @@ RSYNC_VCS_EXCLUSIONS = \
> >  $(BUILD_DIR)/.root:
> >  	mkdir -p $(TARGET_DIR)
> >  	rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
> > -		--chmod=Du+w --exclude .empty --exclude '*~' \
> > +		--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
> >  		$(TARGET_SKELETON)/ $(TARGET_DIR)/
> >  	$(INSTALL) -m 0644 support/misc/target-dir-warning.txt $(TARGET_DIR_WARNING_FILE)
> >  	@ln -snf lib $(TARGET_DIR)/$(LIB_SYMLINK)
> > @@ -612,7 +613,7 @@ endif
> >  	@$(foreach d, $(call qstrip,$(BR2_ROOTFS_OVERLAY)), \
> >  		$(call MESSAGE,"Copying overlay $(d)"); \
> >  		rsync -a --ignore-times $(RSYNC_VCS_EXCLUSIONS) \
> > -			--chmod=Du+w --exclude .empty --exclude '*~' \
> > +			--chmod=u=rwX,go=rX --exclude .empty --exclude '*~' \
> >  			$(d)/ $(TARGET_DIR)$(sep))
> >  
> >  	@$(foreach s, $(call qstrip,$(BR2_ROOTFS_POST_BUILD_SCRIPT)), \
> > diff --git a/support/scripts/shell-wrapper.sh b/support/scripts/shell-wrapper.sh
> > new file mode 100755
> > index 0000000..513b927
> > --- /dev/null
> > +++ b/support/scripts/shell-wrapper.sh
> > @@ -0,0 +1,5 @@
> > +#!/bin/sh
> > +
> > +umask 022
> > +
> > +exec "$@"
> > 
> 
> 
> -- 
> 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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F

Hi Guido,

> Are you sure? I think rsync is used to copy the sysroot to the staging
> dir (output/host). The target is then filled with various 'install -m's
> for each library, so the previous set of permissions wouldn't matter.

The mono package uses rsync to install some libraries in target, so
it's an exception.

Sincerely, Angelo

Hi Angelo,

On Wed, Nov 05, 2014 at 04:51:25PM +0100, Angelo Compagnucci wrote:
> Hi Guido,
> 
> > Are you sure? I think rsync is used to copy the sysroot to the staging
> > dir (output/host). The target is then filled with various 'install -m's
> > for each library, so the previous set of permissions wouldn't matter.
> 
> The mono package uses rsync to install some libraries in target, so
> it's an exception.

I think this is not an issue since we're copying them from output/host,
where we now build with a controlled umask. This means that the current
permissions of source files in the BR repo and the umask won't affect
the outcome. I just built mono with an umask of 077 on a repo with
'chmod -R go=' and I think the end result was ok: 644 for most files,
755 for DLLs and 755 for directories.

Some packages call 'rsync -a' to copy files from their BR directory
(under package/) where we don't track or care about permissions, so
the result is not well-defined. But as long as the source is under
output/host or output/build we should be ok, meaning: we won't depend on
volatile things like the umask or the untracked permissions of the repo.

Of course, you probably know better about mono than I do, so if you do
find a problem let me know!

Thanks!