Skip to content

Conversation

@Flamefire
Copy link
Contributor

@Flamefire Flamefire commented Jul 11, 2024

Description of the Pull Request (PR):

A SquashFS image might contain directory symlinks such as /var/tmp -> /tmp. Before extraction a base environment is created (makeBaseEnv) by pre-creating some directories. Among them /var/tmp. Although unsquashfs is called with -force it fails to replace that by the symlink in the image as it removes only directories.

As per suggestion from @dtrudg the base environment causing the trouble is created after the extraction as done for e.g. OCI images.

This fixes or addresses the following GitHub issues:

A SquashFS image might contain directory symlinks such as `/var/tmp -> /tmp`.
Before extraction a base environment is created (`makeBaseEnv`) by
pre-creating some directories. Among them `/var/tmp`.
Although unsquashfs is called with `-force` it fails to replace that by
the symlink in the image as it removes only directories.

Introduce a pre-extraction step listing the contents of the image and
searching for all entries which are existing directories. Remove those
to avoid the issue.

Fixes sylabs#3084
Copy link
Member

@dtrudg dtrudg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Flamefire - thanks for sharing this. However, it's not the approach we want to take.

  • Relying on scanning the output of unsquashfs is brittle to any changes in that tool, and adds significant complexity to that code. (edit... we are now using squashfs-tools-ng tools elsewhere, and may want to here at some point, so this is why parsing unsquashfs output is additionally an issue)
  • In other code paths, where we build e.g. from an OCI source, rather than a local image file, the directories are created after image extraction, which avoids this issue. We believe we can swap the order of operations for the localimage conveyorpacker code to fix it in a much simpler manner.

I've been discussing this with @cyanezstange and he will be tackling the issue in the next week.

Create a folder to be packed into an image with the following layout:
  /tmp
  /var
    /tmp -> ../tmp
Unpack that to a folder containing the directory /var/tmp
Check that this was replaced by the symlink and other files are
extracted normally
@Flamefire Flamefire force-pushed the fix-symlink-extract branch from 7ebfd9a to 491708f Compare July 12, 2024 07:40
@Flamefire
Copy link
Contributor Author

Relying on scanning the output of unsquashfs is brittle to any changes in that tool, and adds significant complexity to that code. > (edit... we are now using squashfs-tools-ng tools elsewhere, and may want to here at some point, so this is why parsing unsquashfs output is additionally an issue)

I made it as simple as possible with as few assumptions as possible. E.g. using -ls instead of -ll to just get a plain list of names. One issue were warnings output by the singularity sandbox run:

WARNING: Skipping mount /etc/hosts [binds]: /etc/hosts doesn't exist in container
WARNING: Skipping mount /etc/localtime [binds]: /etc/localtime doesn't exist in container
WARNING: Skipping mount /installed/var/singularity/mnt/session/tmp [tmp]: /tmp doesn't exist in container
WARNING: Skipping mount /installed/var/singularity/mnt/session/var/tmp [tmp]: /var/tmp doesn't exist in container
WARNING: Skipping mount /installed/var/singularity/mnt/session/etc/resolv.conf [files]: /etc/resolv.conf doesn't exist in container

So the only assumption I did was that the output of unsquashfs -ls is a list of paths, one per file, in top-down order or at least with the root path first. In case that changes (which I find rather unlikely) then directories might not get removed leading to the same behavior as before. Not too bad IMO.

In other code paths, where we build e.g. from an OCI source, rather than a local image file, the directories are created after image extraction, which avoids this issue. We believe we can swap the order of operations for the localimage conveyorpacker code to fix it in a much simpler manner.

You are right, that is likely better

Create a squashf image unpacked with the following layout:
  /tmp
  /var
    /tmp -> ../tmp
Check that this yields the symlink and other files are extracted normally
A SquashFS image might contain directory symlinks such as `/var/tmp -> /tmp`.
Before extraction a base environment is created (`makeBaseEnv`) by
pre-creating some directories. Among them `/var/tmp`.
Although unsquashfs is called with `-force` it fails to replace that by
the symlink in the image as it removes only directories.

Fix by creating the base env after the extracion as done for e.g. OCI
images.

Fixes sylabs#3084
@cstangeyanez
Copy link
Contributor

Hi @Flamefire , thank you for this PR. I have tidy some thing in #3151 which keep you as an contributor. Please let us know if you don't want to be listed in the CONTRIBUTORS.md file or have any others concerns.

@Flamefire
Copy link
Contributor Author

That's fine for me, thanks!

@Flamefire Flamefire deleted the fix-symlink-extract branch July 29, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error while building from image: "failed to create symlink /image/rootfs/var/tmp"

3 participants