Re: [Buildroot] [PATCH v3 07/16] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

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

Hervé, All,

On 2021-08-17 10:39 +0200, Herve Codina spake thusly:
> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> Without per-package directory support, a package can happily overwrite
> files installed by other packages. Indeed, because the build order
> between packages is always guaranteed, Buildroot will always produce
> the same output.
> 
> However, with per-package directory support, it is absolutely critical
> that a given package does not overwrite files already installed by
> another package, due to how the final aggregation is done to create
> the complete target/, staging/ and host/ folders. Unfortunately, we
> currently don't have anything in Buildroot that detects this
> situation.
> 
> We used to have check-uniq-files, but it was dropped in commit
> 2496189a4207173e4cd5bbab90256f911175ee57.
> 
> This commit is a new implementation of such a detection, which is
> based on calculating and verifying MD5 hashes of installed files: the
> calculation is done at the beginning of the configure step, the
> verification during the newly introduced "install" step that takes
> place after all installation steps.
> 
> Since preventing file overwrites is really only needed when
> per-package directory support is used, and due to this verification
> having some overhead, it is only enabled when
> BR2_PER_PACKAGE_DIRECTORIES=y. This additional verification cost is
> however not too bad as on average, with per-package directory support,
> the per-package target/ and host/ directories will contain less files
> than with a build that doesn't use per-package directory support. This
> helps a bit in mitigating the additional cost of this verification.
> 
> Note that we are not handling separately HOST_DIR and STAGING_DIR,
> like we're doing with the pkg_size_{before,after} functions. Instead,
> the verification on HOST_DIR walks down into the STAGING_DIR.
> 
> During per-package build, original files are modified by
> fixup-libtool-files and fixup-python-files calls.
> But since these fixups modify files using sed --in-place, these
> modifications are done using a temporary file and a call to rename.
> Rename breaks the hardlink to the original file and leave the temporary
> file in per-package TARGET dir.
> As the original file is not modified, this is no longer considered as
> an overwrite. This patch simply considers that what is done by
> fixup-libtool-files and fixup-python-files is part of the original
> snapshot used to detect overwrites. And so, the original snapshot is
> taken after fixup-libtool-files and fixup-python-files calls.

This paragraph should take into consideration the fact that we also now
have those _POST_PREPARE_HOOKS (and first moving the libtool fixups to
be hooks would have simplified this paragraph).

However, what prompted me from applying for now, is that this new
detection is a hard error.

Previously, check-uniq-files was just emitting warnings, but would not
prevent the build from failing. Now, with this patch, even an innocuous
overwrite (e.g. because a post-build script deletes the file, or the
content of the file really does not matter at runtime), the build will
fail.

I.e. configurations that are currently working with PPD, despite the
overwrite, will suddenly no longer build.

OTOH, if we do not make that a hard-error, we will never detect most
issues, because users will never spot those warnings and wil enver
report issues, and the autobuilders will not fail and we will not
notice either...

One solution is to add a configuration knob to make that a hard-error,
like we have the paranoid libs/headers check:

    config BR2_PPD_OVERWRITE_STRICT
        bool "Strict file overwrite detection"
        depends on BR2_PER_PACKAGE_DIRECTORIES
        help
          Say 'y' here to turn the file overwrite detection
          to a hard error. By default, only warnings will be
          printed.

And use that to exit in error only if set:

    [...]
    { \
        echo "ERROR: [...]"; \
        $(if $(BR2_PPD_OVERWRITE_STRICT),exit 1;) \
    } ; \
    [...]

And then, ensure it is always enabled in our autobuilders, so in
utils/genrandconfig:

    diff --git a/utils/genrandconfig b/utils/genrandconfig index 622cfd4891..a891f93b6d 100755
    --- a/utils/genrandconfig
    +++ b/utils/genrandconfig
    @@ -353,6 +353,7 @@ def gen_config(args):
         # Per-package folder
         if randint(0, 15) == 0:
             configlines.append("BR2_PER_PACKAGE_DIRECTORIES=y\n")
    +        configlines.append("BR2_PPD_OVERWRITE_STRICT=y\n")
     
         # Amend the configuration with a few things.
         if randint(0, 20) == 0:

I am not totally sold to the new config knob either, but I'd still
rather have it than unconditionally break the build...

After all, we know that there are some people using PPD (and TLPB) and
are happy with how it works for them, so breaking the build on purpose
would probably not be seen as an improvement...

Regards,
Yann E. MORIN.

> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
> This commit is retreived from Thomas's work.
> The first version was discussed
> https://patchwork.ozlabs.org/project/buildroot/patch/20200430095249.782597-9-thomas.petazzoni@bootlin.com/
> This new version was not already submitted by Thomas or I missed it.
> Compared to the first version, this patch has an improved commit message and
> generates the md5sum snapshot using
>  'LC_ALL=C find $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;'
> instead of
>  'cd $(1); LC_ALL=C find . -type f -exec md5sum {} \; > $($(PKG)_DIR)/.files$(2).md5'
> 
> Changes v1 to v2:
>  - Added note about why fixup-{libtool,python}-files are not considered overwrites
>    and so take the overwrite snapshot after fixup-{libtool,python}-files call.
>  - Removed 'LC_ALL=C'
> 
> Changes v2 to v3:
> None
> 
>  package/pkg-generic.mk | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 38381d93b5..414862e267 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -124,6 +124,25 @@ define FIXUP_PYTHON_SYSCONFIGDATA
>  endef
>  endif
>  
> +# Functions to detect overwritten files
> +
> +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> +# $(1): base directory to search in
> +# $(2): suffix of file (optional)
> +define pkg_detect_overwrite_before
> +	find $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;
> +endef
> +
> +# $(1): base directory to search in
> +# $(2): suffix of file (optional)
> +define pkg_detect_overwrite_after
> +	if test -s $($(PKG)_DIR)/.files$(2).md5 ; then \
> +		md5sum --quiet -c $($(PKG)_DIR)/.files$(2).md5 || \
> +		{ echo "ERROR: package $($(PKG)_NAME) has overwritten files installed by a previous package, aborting."; exit 1; } ; \
> +	fi
> +endef
> +endif
> +
>  # Functions to collect statistics about installed files
>  
>  # $(1): base directory to search in
> @@ -277,6 +296,8 @@ $(BUILD_DIR)/%/.stamp_configured:
>  	$(call fixup-libtool-files,$(NAME),$(HOST_DIR))
>  	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
>  	$(foreach hook,$($(PKG)_POST_PREPARE_HOOKS),$(call $(hook))$(sep))
> +	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
> +	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
>  	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
>  	$($(PKG)_CONFIGURE_CMDS)
>  	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
> @@ -401,6 +422,8 @@ $(BUILD_DIR)/%/.stamp_installed:
>  	@$(call pkg_size_after,$(STAGING_DIR),-staging)
>  	@$(call pkg_size_after,$(HOST_DIR),-host)
>  	@$(call check_bin_arch)
> +	@$(call pkg_detect_overwrite_after,$(TARGET_DIR))
> +	@$(call pkg_detect_overwrite_after,$(HOST_DIR),-host)
>  	$(Q)touch $@
>  
>  # Remove package sources
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/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@busybox.net
http://lists.busybox.net/mailman/listinfo/buildroot