Buildroot Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [RFC/PATCH] target permissions
@ 2014-11-03 12:42 Guido Martínez
  2014-11-03 12:42 ` [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions Guido Martínez
  2014-11-07  3:50 ` [Buildroot] [PATCH v2 0/5] target permissions Guido Martínez
  0 siblings, 2 replies; 15+ messages in thread
From: Guido Martínez @ 2014-11-03 12:42 UTC (permalink / raw)
  To: buildroot

Hi guys,

This patch solves the variablity on target permissions on different
hosts/repos. We should never depend on the umask at any time now.

The default umask was chosen to be 022 since some packages install
binaries on the target rootfs with plain mkdir/cp, so a more permissive
one like 000 would make those binaries world-writable. The --chmod on
rsync behaves similarly to having this same umask.

For the overlay, exceptions to this permission set should be specified
as a device table. The one possible regression I can think of if if
someone uses a pre-build script to set correct permissions on the
overlay before building: those permissions will be reset to 644/755 just
before image creation.

However, if that's not the case I think everything should run smoothly.
Custom permissions set by package makefiles should be respected (unless
overlayed).

Guido Mart?nez (1):
  Makefile: guarantee reproducible permissions

 Makefile                         | 7 ++++---
 support/scripts/shell-wrapper.sh | 5 +++++
 2 files changed, 9 insertions(+), 3 deletions(-)
 create mode 100755 support/scripts/shell-wrapper.sh

-- 
2.1.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions
  2014-11-03 12:42 [Buildroot] [RFC/PATCH] target permissions Guido Martínez
@ 2014-11-03 12:42 ` Guido Martínez
  2014-11-03 22:23   ` Arnout Vandecappelle
  2014-11-07  3:50 ` [Buildroot] [PATCH v2 0/5] target permissions Guido Martínez
  1 sibling, 1 reply; 15+ messages in thread
From: Guido Martínez @ 2014-11-03 12:42 UTC (permalink / raw)
  To: buildroot

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

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 "$@"
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions
  2014-11-03 12:42 ` [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions Guido Martínez
@ 2014-11-03 22:23   ` Arnout Vandecappelle
  2014-11-03 22:31     ` Ezequiel Garcia
  2014-11-05 14:46     ` Guido Martínez
  0 siblings, 2 replies; 15+ messages in thread
From: Arnout Vandecappelle @ 2014-11-03 22:23 UTC (permalink / raw)
  To: buildroot

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 "$@"
> 


-- 
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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions
  2014-11-03 22:23   ` Arnout Vandecappelle
@ 2014-11-03 22:31     ` Ezequiel Garcia
  2014-11-05 14:46     ` Guido Martínez
  1 sibling, 0 replies; 15+ messages in thread
From: Ezequiel Garcia @ 2014-11-03 22:31 UTC (permalink / raw)
  To: buildroot



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.

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions
  2014-11-03 22:23   ` Arnout Vandecappelle
  2014-11-03 22:31     ` Ezequiel Garcia
@ 2014-11-05 14:46     ` Guido Martínez
  2014-11-05 15:51       ` Angelo Compagnucci
  1 sibling, 1 reply; 15+ messages in thread
From: Guido Martínez @ 2014-11-05 14:46 UTC (permalink / raw)
  To: buildroot

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

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions
  2014-11-05 14:46     ` Guido Martínez
@ 2014-11-05 15:51       ` Angelo Compagnucci
  2014-11-07 13:57         ` Guido Martínez
  0 siblings, 1 reply; 15+ messages in thread
From: Angelo Compagnucci @ 2014-11-05 15:51 UTC (permalink / raw)
  To: buildroot

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2 0/5] target permissions
  2014-11-03 12:42 [Buildroot] [RFC/PATCH] target permissions Guido Martínez
  2014-11-03 12:42 ` [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions Guido Martínez
@ 2014-11-07  3:50 ` Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 1/5] Makefile: don't depend on the umask Guido Martínez
                     ` (4 more replies)
  1 sibling, 5 replies; 15+ messages in thread
From: Guido Martínez @ 2014-11-07  3:50 UTC (permalink / raw)
  To: buildroot

Trying again to make the target's permissions well-defined. Differences
with respect to the previous RFC are:

- previous patch splitted into two for clarity

- packages with OVERRIDE_SRCIDR get chmoded so the build doesn't depend
  on the permissions of the source directory

- fixed two packages (matchbox-keyboard and linux-fusion) which copy
  files from the BR directory (for which we don't control permissions)
  with 'cp -p'. Change that to 'install -m' so we don't depend on the
  current status

There is no change for the toolchain, as I think it doesn't has any
issues. The libs are copied to the target via several installs which
set the mode explicitely. As long as toolchain installation is done via
'install', we won't have any problem. However, it's possible that I
missed something :).

Comments are _very_ welcome.

Guido Mart?nez (5):
  Makefile: don't depend on the umask
  Makefile: don't depend on current skeleton/overlay permissions
  pkg-generic.mk: don't depend on external package permissions
  package: matchbox-keyboard: use install instead of cp
  package: linux-fusion: use install instead of cp

 Makefile                                                | 7 ++++---
 package/linux-fusion/linux-fusion.mk                    | 2 +-
 package/matchbox/matchbox-keyboard/matchbox-keyboard.mk | 2 +-
 package/pkg-generic.mk                                  | 2 +-
 support/scripts/shell-wrapper.sh                        | 5 +++++
 5 files changed, 12 insertions(+), 6 deletions(-)
 create mode 100755 support/scripts/shell-wrapper.sh

-- 
2.1.1

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2 1/5] Makefile: don't depend on the umask
  2014-11-07  3:50 ` [Buildroot] [PATCH v2 0/5] target permissions Guido Martínez
@ 2014-11-07  3:50   ` Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 2/5] Makefile: don't depend on current skeleton/overlay permissions Guido Martínez
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Guido Martínez @ 2014-11-07  3:50 UTC (permalink / raw)
  To: buildroot

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.

To fix this, 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.

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

diff --git a/Makefile b/Makefile
index 907a0fc..d9f1fb0 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)
 
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 "$@"
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2 2/5] Makefile: don't depend on current skeleton/overlay permissions
  2014-11-07  3:50 ` [Buildroot] [PATCH v2 0/5] target permissions Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 1/5] Makefile: don't depend on the umask Guido Martínez
@ 2014-11-07  3:50   ` Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 3/5] pkg-generic.mk: don't depend on external package permissions Guido Martínez
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 15+ messages in thread
From: Guido Martínez @ 2014-11-07  3:50 UTC (permalink / raw)
  To: buildroot

We use 'rsync -a' to copy the skeleton and overlays, so the target ends
up with the exact same permissions as on the repo. The problem is we
don't track these permissions, since Git doesn't allow for that (except
for the exec bit). This means users with different umasks at the time of
cloning could end up with different target permissions.

Fix this by using --chmod on rsync calls so we don't depend on the
current permission set for the skeleton and overlays. We do depend on
the exec bit, but that's fine since that one is tracked by Git.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index d9f1fb0..0f7db1e 100644
--- a/Makefile
+++ b/Makefile
@@ -477,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)
@@ -613,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)), \
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2 3/5] pkg-generic.mk: don't depend on external package permissions
  2014-11-07  3:50 ` [Buildroot] [PATCH v2 0/5] target permissions Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 1/5] Makefile: don't depend on the umask Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 2/5] Makefile: don't depend on current skeleton/overlay permissions Guido Martínez
@ 2014-11-07  3:50   ` Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 4/5] package: matchbox-keyboard: use install instead of cp Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 5/5] package: linux-fusion: " Guido Martínez
  4 siblings, 0 replies; 15+ messages in thread
From: Guido Martínez @ 2014-11-07  3:50 UTC (permalink / raw)
  To: buildroot

Reset permissions for rsynced packages (when using OVERRIDE_SRCDIR) to
755/644. We do this under the assumption that source files shouldn't
care about their permissions, except for possibly the exec bit.

This guarantees that if a package uses 'rsync -a' or 'cp -p' to copy
a file from its build dir to the target, it'll end up with the same
permissions on the target every time.

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 package/pkg-generic.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 9643a30..ec2989f 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -117,7 +117,7 @@ $(BUILD_DIR)/%/.stamp_rsynced:
 	@$(call MESSAGE,"Syncing from source dir $(SRCDIR)")
 	@test -d $(SRCDIR) || (echo "ERROR: $(SRCDIR) does not exist" ; exit 1)
 	$(foreach hook,$($(PKG)_PRE_RSYNC_HOOKS),$(call $(hook))$(sep))
-	rsync -au $(RSYNC_VCS_EXCLUSIONS) $(SRCDIR)/ $(@D)
+	rsync -au --chmod=u=rwX,go=rX $(RSYNC_VCS_EXCLUSIONS) $(SRCDIR)/ $(@D)
 	$(foreach hook,$($(PKG)_POST_RSYNC_HOOKS),$(call $(hook))$(sep))
 	$(Q)touch $@
 
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2 4/5] package: matchbox-keyboard: use install instead of cp
  2014-11-07  3:50 ` [Buildroot] [PATCH v2 0/5] target permissions Guido Martínez
                     ` (2 preceding siblings ...)
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 3/5] pkg-generic.mk: don't depend on external package permissions Guido Martínez
@ 2014-11-07  3:50   ` Guido Martínez
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 5/5] package: linux-fusion: " Guido Martínez
  4 siblings, 0 replies; 15+ messages in thread
From: Guido Martínez @ 2014-11-07  3:50 UTC (permalink / raw)
  To: buildroot

in order to not depend on the previous permissions of the file

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 package/matchbox/matchbox-keyboard/matchbox-keyboard.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk b/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
index ebf23e4..ab6dd30 100644
--- a/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
+++ b/package/matchbox/matchbox-keyboard/matchbox-keyboard.mk
@@ -15,7 +15,7 @@ MATCHBOX_KEYBOARD_DEPENDENCIES = host-pkgconf matchbox-lib matchbox-fakekey expa
 MATCHBOX_KEYBOARD_CONF_ENV = expat=yes
 
 define MATCHBOX_KEYBOARD_POST_INSTALL_FIXES
-	cp -dpf ./package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh $(TARGET_DIR)/usr/bin/
+	install -m 0755 ./package/matchbox/matchbox-keyboard/mb-applet-kbd-wrapper.sh $(TARGET_DIR)/usr/bin/
 endef
 
 MATCHBOX_KEYBOARD_POST_INSTALL_TARGET_HOOKS += MATCHBOX_KEYBOARD_POST_INSTALL_FIXES
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2 5/5] package: linux-fusion: use install instead of cp
  2014-11-07  3:50 ` [Buildroot] [PATCH v2 0/5] target permissions Guido Martínez
                     ` (3 preceding siblings ...)
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 4/5] package: matchbox-keyboard: use install instead of cp Guido Martínez
@ 2014-11-07  3:50   ` Guido Martínez
  2014-11-07 12:44     ` Danomi Manchego
  4 siblings, 1 reply; 15+ messages in thread
From: Guido Martínez @ 2014-11-07  3:50 UTC (permalink / raw)
  To: buildroot

in order to not depend on the previous permissions of the file

Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
---
 package/linux-fusion/linux-fusion.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/linux-fusion/linux-fusion.mk b/package/linux-fusion/linux-fusion.mk
index c5e7976..81ecd8a 100644
--- a/package/linux-fusion/linux-fusion.mk
+++ b/package/linux-fusion/linux-fusion.mk
@@ -37,7 +37,7 @@ define LINUX_FUSION_INSTALL_TARGET_CMDS
 		INSTALL_MOD_PATH=$(TARGET_DIR) \
 		-C $(@D) install
 	mkdir -p $(LINUX_FUSION_ETC_DIR)
-	cp -dpf package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)
+	install -m 644 package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)
 endef
 
 $(eval $(generic-package))
-- 
2.1.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2 5/5] package: linux-fusion: use install instead of cp
  2014-11-07  3:50   ` [Buildroot] [PATCH v2 5/5] package: linux-fusion: " Guido Martínez
@ 2014-11-07 12:44     ` Danomi Manchego
  2014-11-07 13:16       ` Guido Martínez
  0 siblings, 1 reply; 15+ messages in thread
From: Danomi Manchego @ 2014-11-07 12:44 UTC (permalink / raw)
  To: buildroot

Guido

On Thu, Nov 6, 2014 at 10:50 PM, Guido Mart?nez
<guido@vanguardiasur.com.ar> wrote:
> in order to not depend on the previous permissions of the file
>
> Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> ---
>  package/linux-fusion/linux-fusion.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/package/linux-fusion/linux-fusion.mk b/package/linux-fusion/linux-fusion.mk
> index c5e7976..81ecd8a 100644
> --- a/package/linux-fusion/linux-fusion.mk
> +++ b/package/linux-fusion/linux-fusion.mk
> @@ -37,7 +37,7 @@ define LINUX_FUSION_INSTALL_TARGET_CMDS
>                 INSTALL_MOD_PATH=$(TARGET_DIR) \
>                 -C $(@D) install
>         mkdir -p $(LINUX_FUSION_ETC_DIR)
> -       cp -dpf package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)
> +       install -m 644 package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)


How about also dropping the mkdir and doing:

       install -D -m 644 package/linux-fusion/40-fusion.rules
$(LINUX_FUSION_ETC_DIR)/40-fusion.rules

Danomi -


>  endef
>
>  $(eval $(generic-package))
> --
> 2.1.1
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [PATCH v2 5/5] package: linux-fusion: use install instead of cp
  2014-11-07 12:44     ` Danomi Manchego
@ 2014-11-07 13:16       ` Guido Martínez
  0 siblings, 0 replies; 15+ messages in thread
From: Guido Martínez @ 2014-11-07 13:16 UTC (permalink / raw)
  To: buildroot

Hi Danomi

On Fri, Nov 07, 2014 at 07:44:50AM -0500, Danomi Manchego wrote:
> Guido
> 
> On Thu, Nov 6, 2014 at 10:50 PM, Guido Mart?nez
> <guido@vanguardiasur.com.ar> wrote:
> > in order to not depend on the previous permissions of the file
> >
> > Signed-off-by: Guido Mart?nez <guido@vanguardiasur.com.ar>
> > ---
> >  package/linux-fusion/linux-fusion.mk | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/package/linux-fusion/linux-fusion.mk b/package/linux-fusion/linux-fusion.mk
> > index c5e7976..81ecd8a 100644
> > --- a/package/linux-fusion/linux-fusion.mk
> > +++ b/package/linux-fusion/linux-fusion.mk
> > @@ -37,7 +37,7 @@ define LINUX_FUSION_INSTALL_TARGET_CMDS
> >                 INSTALL_MOD_PATH=$(TARGET_DIR) \
> >                 -C $(@D) install
> >         mkdir -p $(LINUX_FUSION_ETC_DIR)
> > -       cp -dpf package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)
> > +       install -m 644 package/linux-fusion/40-fusion.rules $(LINUX_FUSION_ETC_DIR)
> 
> 
> How about also dropping the mkdir and doing:
> 
>        install -D -m 644 package/linux-fusion/40-fusion.rules
> $(LINUX_FUSION_ETC_DIR)/40-fusion.rules
Ah yes, that's better. Will do for v3.

Thanks!

> 
> Danomi -
> 
> 
> >  endef
> >
> >  $(eval $(generic-package))
> > --
> > 2.1.1
> >
> > _______________________________________________
> > buildroot mailing list
> > buildroot at busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot

-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions
  2014-11-05 15:51       ` Angelo Compagnucci
@ 2014-11-07 13:57         ` Guido Martínez
  0 siblings, 0 replies; 15+ messages in thread
From: Guido Martínez @ 2014-11-07 13:57 UTC (permalink / raw)
  To: buildroot

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!
-- 
Guido Mart?nez, VanguardiaSur
www.vanguardiasur.com.ar

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2014-11-07 13:57 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-03 12:42 [Buildroot] [RFC/PATCH] target permissions Guido Martínez
2014-11-03 12:42 ` [Buildroot] [RFC/PATCH] Makefile: guarantee reproducible permissions Guido Martínez
2014-11-03 22:23   ` Arnout Vandecappelle
2014-11-03 22:31     ` Ezequiel Garcia
2014-11-05 14:46     ` Guido Martínez
2014-11-05 15:51       ` Angelo Compagnucci
2014-11-07 13:57         ` Guido Martínez
2014-11-07  3:50 ` [Buildroot] [PATCH v2 0/5] target permissions Guido Martínez
2014-11-07  3:50   ` [Buildroot] [PATCH v2 1/5] Makefile: don't depend on the umask Guido Martínez
2014-11-07  3:50   ` [Buildroot] [PATCH v2 2/5] Makefile: don't depend on current skeleton/overlay permissions Guido Martínez
2014-11-07  3:50   ` [Buildroot] [PATCH v2 3/5] pkg-generic.mk: don't depend on external package permissions Guido Martínez
2014-11-07  3:50   ` [Buildroot] [PATCH v2 4/5] package: matchbox-keyboard: use install instead of cp Guido Martínez
2014-11-07  3:50   ` [Buildroot] [PATCH v2 5/5] package: linux-fusion: " Guido Martínez
2014-11-07 12:44     ` Danomi Manchego
2014-11-07 13:16       ` Guido Martínez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox