Re: [Buildroot] [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Luca, All,

On 2022-11-30 14:43 +0100, Luca Pesce via buildroot spake thusly:
> Root makefile imposes 'umask 0022', which means that every file/folder in
> the per-package download directories has no write permission for the group.
> These are writeable just for the owner - the user that issued the first build
> that populated the per-package dl dir for the first time (say user A).
> Thus, if a BR package changes its version (e.g. for buildroot update), and
> another user (say user B, in the same group of A) starts a build, BR fails the
> creation of package-xxx.tar.gz inside the dl dir, because user B has no write
> permissions on that path. Furthermore, in the case of the git backend, this
> makes the git cache not updatable by a different user.
> 
> So, to allow sharing of a rw BR2_DL_DIR location among users of the same group
> on a host machine (e.g. a build server used by many users, all belonging to a
> certain "developers" group), set group write permission to the whole package
> dl dir.
> 
> Signed-off-by: Luca Pesce <luca.pesce@vimar.com>
> ---
>  support/download/dl-wrapper | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
> index 09a6ac1..b7a4319 100755
> --- a/support/download/dl-wrapper
> +++ b/support/download/dl-wrapper
> @@ -190,6 +190,10 @@ main() {
>      new_mode=$(printf "%04o" $((0${new_mode} & ~0$(umask))))

This code was written before we enforced our umask, so it is now indeed
incorrect (at least, the comment above is misleading).

>      chmod ${new_mode} "${tmp_output}"
>  
> +    # Make the whole dl_dir writeable for the group, so other users within
> +    # the group can download new versions and update any vcs cache in it.
> +    chmod -f -R g+w "${dl_dir}"

But what if the user initially had umask 0022 to begin with? By forcing
the group authorization with the chmod, you are overriding the user's
umask settings, which is not good... I for one would not want to have
group-writable directories (or files) created when I would have not
expected it.

Instead, what about something like:

    diff --git a/Makefile b/Makefile
    index 827ab230ef..949f27f1eb 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -83,10 +83,15 @@ $(MAKECMDGOALS): _all
     _all:
     	@umask $(REQ_UMASK) && \
     		$(MAKE) -C $(CANONICAL_CURDIR) --no-print-directory \
    +			BR_ORIG_UMASK=$(shell umask) \
     			$(MAKECMDGOALS) $(EXTRAMAKEARGS)
     
     else # umask / $(CURDIR) / $(O)
     
    +# Save the user's original umask, as we may need it later on, like
    +# during the download dl-wrapper
    +export BR_ORIG_UMASK := $(or $(BR_ORIG_UMASK),$(shell umask))
    +
     # This is our default rule, so must come first
     all:
     .PHONY: all
    diff --git a/package/pkg-download.mk b/package/pkg-download.mk
    index 0718f21aad..e9dfec635d 100644
    --- a/package/pkg-download.mk
    +++ b/package/pkg-download.mk
    @@ -107,9 +107,11 @@ endif
     #
     ################################################################################
     
    +# Use user's original umask, in case they have provisions set to share
    +# the download directory with their group (or the whole world).
     define DOWNLOAD
    -$(Q)mkdir -p $($(2)_DL_DIR)
    -		$(Q)$(EXTRA_ENV) $($(2)_DL_ENV) \
    +	$(Q)umask $(BR_ORIG_UMASK); mkdir -p $($(2)_DL_DIR)
    +	$(Q)umask $(BR_ORIG_UMASK); $(EXTRA_ENV) $($(2)_DL_ENV) \
     		flock $($(2)_DL_DIR)/.lock $(DL_WRAPPER) \
     		-c '$($(2)_DL_VERSION)' \
     		-d '$($(2)_DL_DIR)' \

The comments are only mostly stubs and need being expanded a bit...

Regards,
Yann E. MORIN.

>      # We must *not* unlink tmp_output, otherwise there is a small window
>      # during which another download process may create the same tmp_output
>      # name (very, very unlikely; but not impossible.)
> -- 
> 2.7.4
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot