core/sdk: fix relative symlinks in generated tarball (branch yem/sdk)

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 symlinl pointing out of the base directory, because their targets would be missing from the SDK.

Signed-off-by "Yann E. MORIN" <[email protected]>

Cc Joel Carlson <[email protected]>

From: Joel Carlson <[email protected]>

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 <[email protected]>

[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" <[email protected]>

Quoted 30 lines

From: Joel Carlson <[email protected]>
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.

Quoted 8 lines

Signed-off-by Joel Carlson <[email protected]>

[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" <[email protected]>

So not sure what protocol is on tags when this is essentially v2 of my own patch, but...

Reviewed-by Joel Carlson <[email protected]>

Tested-by Joel Carlson <[email protected]>

Applied this and the first patch to my branch and made sure my SDKs seemed to have the correct links.

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 acb89c8d93952468fc9e24c7ddd9e361b8108e07

   .gitlab-ci.yml: regenerate after prosody tests addition (2018-12-06 23:15:20 +0100)

are available in the git repository at:

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

for you to fetch changes up to 6ce3997e4824d41b50e13b8554e9f7e59e63a8af

   core/sdk: don't mangle symlinks with '.' or '..' at start (2018-12-07 18:59:41 +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 | 53 ++++++++++++++++++++++++++++++++++++++++++++
  2 files changed, 56 insertions(+), 2 deletions(-)

create mode 100755 support/scripts/fix-symlinks

However, the check for symlinks pointing out of host/ showed to two issues originating in install steps of iptables and eudev. Both create links that point to system pathes.

I have locally made POST_STAGING_HOOKS to fix that but that's probably a non acceptable hack.

Quoted 6 lines

However, the check for symlinks pointing out of host/ showed to two issues originating in install steps of iptables and eudev. Both create links that point to system pathes.

I have locally made POST_STAGING_HOOKS to fix that but that's probably a non acceptable hack.

Would be hard to tell without seeing the actual change. ;-) That being said, this is probably something that would need to be fixed in iptables and eudev (patches submitted upstream), and then those patches added to buildroot to fix the issue.

So far I've submitted a patch to iptables here:
http://patchwork.ozlabs.org/patch/1017739/

And a patch to buildroot to pull in a version of that here:
https://patchwork.ozlabs.org/patch/1017740/

I've also submitted a pull request for eudev (and while typing this email it was already merged):
https://github.com/gentoo/eudev/pull/165

I'll submit a patch for eudev in buildroot to pull in that change once I see feedback on the iptables one. For it to work for me I needed to set AUTORECONF=YES on eudev, but I don't really understand autotools/automake stuff very well and I don't know if that is the right way to be doing things.
-Joel

Joel, Andreas, All,

On 2018-12-21 17:13 -0700, Joel Carlson spake thusly:

Quoted 19 lines · Andreas Naumann

Quoted 6 lines

However, the check for symlinks pointing out of host/ showed to two issues originating in install steps of iptables and eudev. Both create links that point to system pathes.

I have locally made POST_STAGING_HOOKS to fix that but that's probably a non acceptable hack.

Would be hard to tell without seeing the actual change. ;-) That being said, this is probably something that would need to be fixed in iptables and eudev (patches submitted upstream), and then those patches added to buildroot to fix the issue.

So far I've submitted a patch to iptables here:
http://patchwork.ozlabs.org/patch/1017739/

And a patch to buildroot to pull in a version of that here:
https://patchwork.ozlabs.org/patch/1017740/

Quoted 3 lines

I've also submitted a pull request for eudev (and while typing this email it was already merged):
https://github.com/gentoo/eudev/pull/165

I did nt see Thomas' reply to the iptables patch before I replied to your eudev one. It turns out we basically said exactly the same thing:
this is not the correct solution.

If you do what you propose, then you can see that at runtime, on the _target_, the symlinks will be wrong, bnecause they would be prefixed with the DESTDIR, which is clearly not appropriate.

A better solution, in that case, wuld be to make relative symlinks instead, but that is a little bit trickier. A simple way to do that is to use 'ln -r -s FILE LINK' but the -r option is not old enough to be widely available on all distors, especially entreprise-grade distros.

An other approach, would be that we whietelist certain PATHs from our SDK. Afterall, there are things we do not care about in the SDK. For example, most executables in staging/{,usr/}{,s}bin are not needed. We only really need libs and headers, so we could just ignore anything that is not in staging/{lib,usr/lib,usr/include}/

I.e. the heuristic would become:

if it is a symlink:
if is not below staging/:
check and fix it
else if it is below staging/{lib,usr/lib,usr/include}:
check and fix it
else
ignore it

I can try to look into fixing that in my series. Is enabling eudev and/or iptables enough to echibit the problem?

Regards,
Yann E. MORIN.

Quoted 6 lines

I'll submit a patch for eudev in buildroot to pull in that change once I see feedback on the iptables one. For it to work for me I needed to set AUTORECONF=YES on eudev, but I don't really understand autotools/automake stuff very well and I don't know if that is the right way to be doing things.
-Joel

--
.-----------------.--------------------.------------------.--------------------.

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

'------------------------------^-------^------------------^--------------------'

Quoted 3 lines

I did nt see Thomas' reply to the iptables patch before I replied to your eudev one. It turns out we basically said exactly the same thing:
this is not the correct solution.

My apologies on submitting a bad patch.

Quoted 8 lines

if it is a symlink:
if is not below staging/:
check and fix it
else if it is below staging/{lib,usr/lib,usr/include}:
check and fix it
else
ignore it

Quoted 2 lines

I can try to look into fixing that in my series. Is enabling eudev and/or iptables enough to echibit the problem?

That is a better solution. And yes, with the previous sdk fixes applied, just enabling eudev and/or iptables and then trying to 'make sdk' would error out because of the symlink path being outside of staging.

Quoted 7 lines · Yann E. MORIN

An other approach, would be that we whietelist certain PATHs from our SDK. Afterall, there are things we do not care about in the SDK. For example, most executables in staging/{,usr/}{,s}bin are not needed. We only really need libs and headers, so we could just ignore anything that is not in staging/{lib,usr/lib,usr/include}/

Am 07.12.18 um 19:10 schrieb Yann E. MORIN:

Quoted 11 lines

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.

Thank you for these patches as they adress an SDK issue I have just found. I have applied them on top of 2018.11 and verified the link ./usr -> ./ remains as is and is not changed into a dead link to ./arm-buildroot-linux-gnueabihf_sdk-buildroot/.

However, the check for symlinks pointing out of host/ showed to two issues originating in install steps of iptables and eudev. Both create links that point to system pathes.

I have locally made POST_STAGING_HOOKS to fix that but that's probably a non acceptable hack.