[Buildroot] [PATCH 0/2 v3] core/sdk: fix relative symlinks in generated tarball (branch yem/sdk)

[Buildroot] [PATCH 0/2 v3] core/sdk: fix relative symlinks in generated tarball (branch yem/sdk)

[Yann E. MORIN]

Hello All!

This two-patch series is my counter-proposal to Joel's path to try and
fix the issue:
    https://patchwork.ozlabs.org/patch/1009168/

The first patch fixes the absolute symlinks, that were so far not
accounted for at all. A new helper scripts changes them into relative
symlinks.

The second patch is the actual fix to the issue Joel's patch was trying
to solve. I believe it is a better and simpler solution, although I have
to admit the patsubst trickery is not obvious.


Regards,
Yann E. MORIN.


The following changes since commit ee58353462f32f46f3f5b93f6dc78e94b31bad78

  DEVELOPERS: remove Vicente Olivert Riera (2018-12-22 08:10:03 +0100)


are available in the git repository at:

  git://git.buildroot.org/~ymorin/git/buildroot.git

for you to fetch changes up to 235301274e7a921bb0b98aa606403b12fbee08be

  core/sdk: don't mangle symlinks with '.' or '..' at start (2018-12-22 13:55:04 +0100)


----------------------------------------------------------------
Joel Carlson (1):
      core/sdk: don't mangle symlinks with '.' or '..' at start

Yann E. MORIN (1):
      core: make symlinks relative when preparing the SDK

 Makefile                     |  5 ++--
 support/scripts/fix-symlinks | 71 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100755 support/scripts/fix-symlinks

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/2 v3] core: make symlinks relative when preparing the SDK

[Yann E. MORIN]

The SDK is supposed to be relocatable, so symlinks must not be
absolute.

Add a new helper script, that replaces all absolute symlinks with
relative ones.

The ideal solution would use the shortest relative path for the
destination, but it is non-trivial to come up with. We just ensure
the relative path does not cross a common base directory, and compute
all the paths relative to that anchor.

This can give non-optimum relative symlinks, like:
    /base-dir/bin/foo -> ../bin/bar

when the optimum would have been:
    /base-dir/bin/foo -> bar

Finally, as a sanity check, ensure there is not relative symlink
pointing out of the base directory, because their targets would be
missing from the SDK.

We do one exception to that last rule: any symlink in a bin directory
of staging, that point s to a bin directory of the host, is simply
ignored. They are supposed to be validate on the target, and so are
supposed to point to target executables, so we are not going to execute
them anyway (we're still going to execute the foo-config scripts, but
they are not symlinks, so we're OK).

Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Cc: Joel Carlson <JoelsonCarl@gmail.com>
Cc: Andreas Naumann <dev@andin.de>

---
Changes v2 -> v3:
  - account for absolute symlinks in staging's bin dirs  (Andreas)
---
 Makefile                     |  1 +
 support/scripts/fix-symlinks | 71 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)
 create mode 100755 support/scripts/fix-symlinks

diff --git a/Makefile b/Makefile
index c5b78b3274..e1b58e7bc3 100644
--- a/Makefile
+++ b/Makefile
@@ -586,6 +586,7 @@ prepare-sdk: world
 	@$(call MESSAGE,"Rendering the SDK relocatable")
 	$(TOPDIR)/support/scripts/fix-rpath host
 	$(TOPDIR)/support/scripts/fix-rpath staging
+	$(TOPDIR)/support/scripts/fix-symlinks
 	$(INSTALL) -m 755 $(TOPDIR)/support/misc/relocate-sdk.sh $(HOST_DIR)/relocate-sdk.sh
 	mkdir -p $(HOST_DIR)/share/buildroot
 	echo $(HOST_DIR) > $(HOST_DIR)/share/buildroot/sdk-location
diff --git a/support/scripts/fix-symlinks b/support/scripts/fix-symlinks
new file mode 100755
index 0000000000..68c45fed6d
--- /dev/null
+++ b/support/scripts/fix-symlinks
@@ -0,0 +1,71 @@
+#!/usr/bin/env bash
+# SPDX-License-Identifier: GPL-2.0-or-later
+
+# Transform all symlinks in ${HOST_DIR} from absolute to relative,
+# with the base of relativity anchored in ${HOST_DIR}
+#
+# Additionally, ensure that all existing symlinks point inside
+# ${HOST_DIR}. As an exception, absolute symlinks in a bin directory
+# of ${STAGING_DIR}, that point to one of the host's bin direcotry
+# are simply ignored, on the assumption that they are executables for
+# target, but we won't run them (except some foo-config script, but
+# those are not symlinks).
+#
+# Note that the relativity of the symlink is not the shortest, e.g.
+# ${HOST_DIR}/bin/foo can end up as a symlink to ../bin/bar when it
+# would be shorter to have a symlink to ./bar, or even simply to bar.
+# This is not the nicest, but works and is the easiest to do.
+#
+# Environment:
+#   HOST_DIR     host directory
+#   STAGING_DIR  staging directory
+#
+# Returns:
+#   0 when symlinks are OK (whether fixups were done or not);
+#   1 when at least one symlink is pointing outside of HOST_DIR and
+#     can't be fixed
+
+main() {
+    local link dest reldir
+    local -a links
+
+    links=( $(find "${HOST_DIR}" -type l -printf '%P\n') )
+    for link in "${links[@]}"; do
+        dest="$(readlink "${HOST_DIR}/${link}")"
+        case "${dest}" in
+          ("${HOST_DIR}"/*) # Absolute symlink below HOST_DIR, needs fixup
+            reldir="$(sed -r -e 's,([^/]+/),../,g; s,/[^/]+$,,' <<<"${link#${HOST_DIR}}")"
+            rm -f "${HOST_DIR}/${link}"
+            ln -sf "${reldir}${dest#${HOST_DIR}}" "${HOST_DIR}/${link}"
+            ;;
+          (/*) # Absolute symlink outside of HOST_DIR
+            if [[    "${link}" =~ ^${STAGING_DIR#${HOST_DIR}/}/(usr/)?s?bin/ \
+                  && "${dest}" =~ ^/(usr/)?s?bin/ ]]
+            then
+                # In one of the bin dirs of STAGING_DIR, pointing to a bin
+                # dir of the host, ignore (we won't need to run them)
+                continue
+            fi
+            # Any other symlink pointing outside of HOST_DIR, not supported
+            printf 'Absolute symlink outside of HOST_DIR "%s": "%s" -> "%s"\n' \
+                   "${HOST_DIR}" "${link}" "${dest}"
+            exit 1
+            ;;
+          (../*|*/../*|*/..) # Not absolute, but does it cross the boundary?
+            dest="$(readlink -m "${HOST_DIR}/${link}")"
+            case "${dest}" in
+              ("${HOST_DIR}"/*) ;; # Anchored, OK
+              (*)
+                printf 'Relative symlink outside of HOST_DIR "%s": "%s" -> "%s"\n' \
+                       "${HOST_DIR}" "${link}" "${dest}"
+                exit 1
+                ;;
+            esac
+            ;;
+          (*) # Not absolute, does not cross, OK
+            ;;
+        esac
+    done
+}
+
+main "${@}"
-- 
2.14.1

[Buildroot] [PATCH 1/2 v3] core: make symlinks relative when preparing the SDK

[Thomas Petazzoni]

Hello,

On Sat, 22 Dec 2018 16:18:51 +0100, Yann E. MORIN wrote:
> The SDK is supposed to be relocatable, so symlinks must not be
> absolute.
> 
> Add a new helper script, that replaces all absolute symlinks with
> relative ones.

This script is really not trivial. This additional complexity raises a
simple question: how many packages install such absolute symlinks?
Should we instead detect such absolute symlinks, and use that to fix
the problematic packages ?

I am not saying at all that the approach of fixing all symlinks is
wrong, but I'd like to see a proper discussion on the balance between
fixing the problematic packages vs. taking the "brutal" approach of
this new fix-symlinks script.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 1/2 v3] core: make symlinks relative when preparing the SDK

[Joel Carlson]

On Wed, Dec 26, 2018 at 2:21 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Sat, 22 Dec 2018 16:18:51 +0100, Yann E. MORIN wrote:
> > The SDK is supposed to be relocatable, so symlinks must not be
> > absolute.
> >
> > Add a new helper script, that replaces all absolute symlinks with
> > relative ones.
>
> This script is really not trivial. This additional complexity raises a
> simple question: how many packages install such absolute symlinks?

That was actually a good question.  I did a build where I turned on
some 450+ random packages (estimated by counting the folders in build/
other than host-* folders).  I encountered the following absolute
links:
iptables - 1 absolute symlink (sysroot/usr/bin/iptables-xml ->
/usr/sbin/xtables-multi)
ntfs-3g - 2 absolute symlinks (sysroot/sbin/mount.ntfs-3g ->
/usr/bin/ntfs-3g  and  sysroot/sbin/mount.lowntfs-3g ->
/usr/bin/lowntfs-3g)
fontconfig - 16 absolute symlinks (sysroot/etc/fonts/conf.d/* ->
/usr/share/fontconfig/conf.avail/*)
And then not in my build, but pointed out by someone on a previous
email thread, the eudev package produces 1 absolute symlink

I could dump my actual config somewhere if desired.

> Should we instead detect such absolute symlinks, and use that to fix
> the problematic packages ?
>
> I am not saying at all that the approach of fixing all symlinks is
> wrong, but I'd like to see a proper discussion on the balance between
> fixing the problematic packages vs. taking the "brutal" approach of
> this new fix-symlinks script.

So with such a small amount of absolute symlinks so far, and all of
them being for things we generally don't care about in the SDK, then
perhaps just check for absolute symlinks in a whitelist of areas like
/lib (or perhaps blacklist areas like /etc or /usr/bin).  Error out on
an absolute symlink in the spots we care about, and fix that upstream?

The thought behind the whitelist/blacklist as I understand it is
because I tried fixing iptables and eudev, but did it incorrectly.
The discussion on my eudev change [1] makes it sound a bit harder to
solve (relies on ln command supporting "-r" which may not be
everywhere), but being a link we don't care about in the SDK, we could
just ignore it with a whitelist/blacklist.

[1] https://github.com/gentoo/eudev/pull/165




>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

[Buildroot] [PATCH 1/2 v3] core: make symlinks relative when preparing the SDK

[Andreas Naumann]

Hi,

Am 05.01.19 um 01:53 schrieb Joel Carlson:
> On Wed, Dec 26, 2018 at 2:21 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
>>
>> Hello,
>>
>> On Sat, 22 Dec 2018 16:18:51 +0100, Yann E. MORIN wrote:
>>> The SDK is supposed to be relocatable, so symlinks must not be
>>> absolute.
>>>
>>> Add a new helper script, that replaces all absolute symlinks with
>>> relative ones.
>>
>> This script is really not trivial. This additional complexity raises a
>> simple question: how many packages install such absolute symlinks?
> 
> That was actually a good question.  I did a build where I turned on
> some 450+ random packages (estimated by counting the folders in build/
> other than host-* folders).  I encountered the following absolute
> links:
> iptables - 1 absolute symlink (sysroot/usr/bin/iptables-xml ->
> /usr/sbin/xtables-multi)
> ntfs-3g - 2 absolute symlinks (sysroot/sbin/mount.ntfs-3g ->
> /usr/bin/ntfs-3g  and  sysroot/sbin/mount.lowntfs-3g ->
> /usr/bin/lowntfs-3g)
> fontconfig - 16 absolute symlinks (sysroot/etc/fonts/conf.d/* ->
> /usr/share/fontconfig/conf.avail/*)
> And then not in my build, but pointed out by someone on a previous
> email thread, the eudev package produces 1 absolute symlink

I've been toying with this patch as well and among ~100 packages also 
found the absolute symlinks in iptables and eudev (only). Short of time 
and knowledge of how to suppress them being created I made two patches 
in the form:

+define EUDEV_REMOVE_BROKEN_SYMLINK
+	$(RM) $(STAGING_DIR)/sbin/udevadm
+endef
+
+EUDEV_POST_INSTALL_STAGING_HOOKS += EUDEV_REMOVE_BROKEN_SYMLINK
+

However, I don't propose this as a solution as I think a blacklist would 
concentrate the issue in one place rather than spreading exceptions all 
over the place.


regards,
Andreas

[Buildroot] [PATCH 2/2 v3] core/sdk: don't mangle symlinks with '.' or '..' at start

[Yann E. MORIN]

From: Joel Carlson <JoelsonCarl@gmail.com>

The current transform changes any '.' at the start of a filename to
$(BR2_SDK_PREFIX). This also applies to the target of a symlink, when
it is relative.

We thus might end up with something like:
    $(BR2_SDK_PREFIX)/bin/aarch64-linux-gnu-ar ->
    $(BR2_SDK_PREFIX)./opt/ext-toolchain/bin/aarch64-linux-gnu-ar

when it should be:
    $(BR2_SDK_PREFIX)/bin/aarch64-linux-gnu-ar ->
    ../opt/ext-toolchain/bin/aarch64-linux-gnu-ar

We fix that by making sure we always remove a known prefix, i.e. we
remove the path to host dir. The obvious solution would be to cd into
$(HOST_DIR)/.. , then tar ./host/ and finally use a --transfrom pattern
as 's,^\./$(notdir $(HOST_DIR)),$(BR2_SDK_PREFIX)'.

Since $(HOST_DIR) can point to a user-supplied location, we don't know
very well how the pattern may patch.

Instead, we cd into / and tar the full path to $(HOST_DIR).

Since tar removes any leading '/', it would spurr a warning message,
which is annoying. So we explicitly remove the leading '/' from
$(HOST_DIR) when we tar it.

Finally, we transform all filenames to replace a leading $(HOST_DIR)
(without a leading /) to the prefix to use.

Signed-off-by: Joel Carlson <JoelsonCarl@gmail.com>
[yann.morin.1998 at free.fr:
  - use a single transform pattern
  - use full HOST_DIR path as pattern to replace
  - update commit log accordingly
]
Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

---
Changes v1 -> v2: (Yann)
  - changes in the commit log, above.
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index e1b58e7bc3..8fc7984006 100644
--- a/Makefile
+++ b/Makefile
@@ -599,8 +599,8 @@ sdk: prepare-sdk $(BR2_TAR_HOST_DEPENDENCY)
 	$(Q)mkdir -p $(BINARIES_DIR)
 	$(TAR) czf "$(BINARIES_DIR)/$(BR2_SDK_PREFIX).tar.gz" \
 		--owner=0 --group=0 --numeric-owner \
-		--transform='s#^\.#$(BR2_SDK_PREFIX)#' \
-		-C $(HOST_DIR) "."
+		--transform='s#^$(patsubst /%,%,$(HOST_DIR))#$(BR2_SDK_PREFIX)#' \
+		-C / $(patsubst /%,%,$(HOST_DIR))
 
 RSYNC_VCS_EXCLUSIONS = \
 	--exclude .svn --exclude .git --exclude .hg --exclude .bzr \
-- 
2.14.1

[Buildroot] [PATCH 2/2 v3] core/sdk: don't mangle symlinks with '.' or '..' at start

[Arnout Vandecappelle]

On 22/12/2018 16:18, Yann E. MORIN wrote:
> From: Joel Carlson <JoelsonCarl@gmail.com>
> 
> The current transform changes any '.' at the start of a filename to
> $(BR2_SDK_PREFIX). This also applies to the target of a symlink, when
> it is relative.
> 
> We thus might end up with something like:
>     $(BR2_SDK_PREFIX)/bin/aarch64-linux-gnu-ar ->
>     $(BR2_SDK_PREFIX)./opt/ext-toolchain/bin/aarch64-linux-gnu-ar
> 
> when it should be:
>     $(BR2_SDK_PREFIX)/bin/aarch64-linux-gnu-ar ->
>     ../opt/ext-toolchain/bin/aarch64-linux-gnu-ar
> 
> We fix that by making sure we always remove a known prefix, i.e. we
> remove the path to host dir. The obvious solution would be to cd into
> $(HOST_DIR)/.. , then tar ./host/ and finally use a --transfrom pattern
> as 's,^\./$(notdir $(HOST_DIR)),$(BR2_SDK_PREFIX)'.
> 
> Since $(HOST_DIR) can point to a user-supplied location, we don't know
> very well how the pattern may patch.
> 
> Instead, we cd into / and tar the full path to $(HOST_DIR).
> 
> Since tar removes any leading '/', it would spurr a warning message,
> which is annoying. So we explicitly remove the leading '/' from
> $(HOST_DIR) when we tar it.
> 
> Finally, we transform all filenames to replace a leading $(HOST_DIR)
> (without a leading /) to the prefix to use.
> 
> Signed-off-by: Joel Carlson <JoelsonCarl@gmail.com>
> [yann.morin.1998 at free.fr:
>   - use a single transform pattern
>   - use full HOST_DIR path as pattern to replace
>   - update commit log accordingly
> ]
> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

 Applied to master, thanks.

 Regards,
 Arnout

> 
> ---
> Changes v1 -> v2: (Yann)
>   - changes in the commit log, above.
> ---
>  Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index e1b58e7bc3..8fc7984006 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -599,8 +599,8 @@ sdk: prepare-sdk $(BR2_TAR_HOST_DEPENDENCY)
>  	$(Q)mkdir -p $(BINARIES_DIR)
>  	$(TAR) czf "$(BINARIES_DIR)/$(BR2_SDK_PREFIX).tar.gz" \
>  		--owner=0 --group=0 --numeric-owner \
> -		--transform='s#^\.#$(BR2_SDK_PREFIX)#' \
> -		-C $(HOST_DIR) "."
> +		--transform='s#^$(patsubst /%,%,$(HOST_DIR))#$(BR2_SDK_PREFIX)#' \
> +		-C / $(patsubst /%,%,$(HOST_DIR))
>  
>  RSYNC_VCS_EXCLUSIONS = \
>  	--exclude .svn --exclude .git --exclude .hg --exclude .bzr \
> 

[Buildroot] [PATCH 2/2 v3] core/sdk: don't mangle symlinks with '.' or '..' at start

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 > On 22/12/2018 16:18, Yann E. MORIN wrote:
 >> From: Joel Carlson <JoelsonCarl@gmail.com>
 >> 
 >> The current transform changes any '.' at the start of a filename to
 >> $(BR2_SDK_PREFIX). This also applies to the target of a symlink, when
 >> it is relative.
 >> 
 >> We thus might end up with something like:
 >> $(BR2_SDK_PREFIX)/bin/aarch64-linux-gnu-ar ->
 >> $(BR2_SDK_PREFIX)./opt/ext-toolchain/bin/aarch64-linux-gnu-ar
 >> 
 >> when it should be:
 >> $(BR2_SDK_PREFIX)/bin/aarch64-linux-gnu-ar ->
 >> ../opt/ext-toolchain/bin/aarch64-linux-gnu-ar
 >> 
 >> We fix that by making sure we always remove a known prefix, i.e. we
 >> remove the path to host dir. The obvious solution would be to cd into
 >> $(HOST_DIR)/.. , then tar ./host/ and finally use a --transfrom pattern
 >> as 's,^\./$(notdir $(HOST_DIR)),$(BR2_SDK_PREFIX)'.
 >> 
 >> Since $(HOST_DIR) can point to a user-supplied location, we don't know
 >> very well how the pattern may patch.
 >> 
 >> Instead, we cd into / and tar the full path to $(HOST_DIR).
 >> 
 >> Since tar removes any leading '/', it would spurr a warning message,
 >> which is annoying. So we explicitly remove the leading '/' from
 >> $(HOST_DIR) when we tar it.
 >> 
 >> Finally, we transform all filenames to replace a leading $(HOST_DIR)
 >> (without a leading /) to the prefix to use.
 >> 
 >> Signed-off-by: Joel Carlson <JoelsonCarl@gmail.com>
 >> [yann.morin.1998 at free.fr:
 >> - use a single transform pattern
 >> - use full HOST_DIR path as pattern to replace
 >> - update commit log accordingly
 >> ]
 >> Signed-off-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Committed to 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard