Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make --symlink idempotent, or introduce a new --symlink-idempotent #549

Closed
smcv opened this issue Jan 12, 2023 · 2 comments
Closed

Make --symlink idempotent, or introduce a new --symlink-idempotent #549

smcv opened this issue Jan 12, 2023 · 2 comments

Comments

@smcv
Copy link
Collaborator

smcv commented Jan 12, 2023

We've had several Flatpak issues of this general form:

  • Flatpak bind-mounts some directory /dir into the sandbox (read-only or read/write)
  • Flatpak also wants to set up some symlink /dir/symlink -> target below that directory
  • The symlink already exists in the bind-mounted directory
  • ... so bwrap --symlink target /dir/symlink fails with EEXIST

or this:

  • Flatpak explicitly creates a symlink /var/run -> /run
  • The host system also has a symlink /var/run -> /run
  • Flatpak has been asked to share some filesystem with the container which happens to be below /var/run, like /var/run/media, which it does by creating the symlink /var/run -> /run and mounting /run/media onto the sandbox's /run/media
  • ... so it ends up with bwrap --symlink /run /var/run ... --symlink /run /var/run which fails with EEXIST

I see two ways this could be addressed.

The most general solution would be for Flatpak (and other projects that use bwrap) to be more aware of what it is mounting and how, so that if it wants to create a symlink below a directory it is sharing with the sandbox, it creates it in the source directory on the host instead of asking bwrap to create it below the destination mount point. Flatpak already has FlatpakExports, which is aware of every --filesystem that it is going to share with the sandbox, but is currently unaware of things like --persist, leading to bugs; so this solution would involve extending FlatpakExports to be aware of more locations. However, this general solution is going to be quite complicated.

A 90% solution would be to make bwrap --symlink idempotent, or leave bwrap --symlink as-is for bug-for-bug compatibility but introduce a new --symlink-idempotent (or an --idempotent modifier to be used like --size, or something). How this would work:

  • if the symlink doesn't already exist in /newroot, still create it, like --symlink
  • if the symlink already exists in /newroot, and it has the same target we want to create, new behaviour: do nothing
  • if the symlink already exists in /newroot, and it has a different target but works out to be equivalent (like we want /var/run -> /run when /var/run -> ../run exists), maybe new behaviour: also do nothing (I am unsure whether this is desirable or not)
  • if the desired path already exists in /newroot but is not a symlink, or is a symlink pointing somewhere different, still fail like --symlink would
@smcv
Copy link
Collaborator Author

smcv commented Jan 12, 2023

jkaivo added a commit to jkaivo/bubblewrap that referenced this issue Apr 21, 2023
When creating symlinks specified with --symlink, check errno if
creation fails. If the reason was EEXIST, check the path of the
existing symlink. If it matches what was desired, continue as
normal.

Fixes containers#549

Signed-off-by: Jakob Kaivo <jkk@ung.org>
@jkaivo
Copy link
Contributor

jkaivo commented Apr 21, 2023

Pull request #577 submitted that ignores failure to create symlinks if the existing target matches the desired target. It does not attempt to resolve equivalence, only exact matches.

smcv pushed a commit to jkaivo/bubblewrap that referenced this issue Jan 31, 2024
When creating symlinks specified with --symlink, check errno if
creation fails. If the reason was EEXIST, check the path of the
existing symlink. If it matches what was desired, continue as
normal.

[Anders Blomdell: use readlink_malloc()]
[smcv: Squash multiple changes into one; coding style fixes]
Resolves: containers#549
Signed-off-by: Jakob Kaivo <jkk@ung.org>
Co-authored-by: Anders Blomdell
Co-authored-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
@smcv smcv closed this as completed in 4109d59 Jan 31, 2024
smcv added a commit to smcv/flatpak that referenced this issue Mar 27, 2024
* `--symlink` is now idempotent, meaning it succeeds if the
  symlink already exists and already has the desired target
  (containers/bubblewrap#549, flatpak#2387,
  flatpak#3477, flatpak#5255)
* Report a better error message if `mount(2)` fails with `ENOSPC`
  (containers/bubblewrap#615, ValveSoftware/steam-runtime#637)
* Fix a double-close on error reading from `--args`, `--seccomp` or
  `--add-seccomp-fd` argument (containers/bubblewrap#558)
* Improve memory allocation behaviour
  (containers/bubblewrap#556, containers/bubblewrap#624)
* Silence various compiler warnings (containers/bubblewrap#559)

Resolves: flatpak#2387
Resolves: flatpak#3477
Resolves: flatpak#5255
Signed-off-by: Simon McVittie <smcv@collabora.com>
smcv added a commit to flatpak/flatpak that referenced this issue Mar 27, 2024
* `--symlink` is now idempotent, meaning it succeeds if the
  symlink already exists and already has the desired target
  (containers/bubblewrap#549, #2387,
  #3477, #5255)
* Report a better error message if `mount(2)` fails with `ENOSPC`
  (containers/bubblewrap#615, ValveSoftware/steam-runtime#637)
* Fix a double-close on error reading from `--args`, `--seccomp` or
  `--add-seccomp-fd` argument (containers/bubblewrap#558)
* Improve memory allocation behaviour
  (containers/bubblewrap#556, containers/bubblewrap#624)
* Silence various compiler warnings (containers/bubblewrap#559)

Resolves: #2387
Resolves: #3477
Resolves: #5255
Signed-off-by: Simon McVittie <smcv@collabora.com>
GeorgesStavracas pushed a commit to GeorgesStavracas/flatpak that referenced this issue Apr 26, 2024
* `--symlink` is now idempotent, meaning it succeeds if the
  symlink already exists and already has the desired target
  (containers/bubblewrap#549, flatpak#2387,
  flatpak#3477, flatpak#5255)
* Report a better error message if `mount(2)` fails with `ENOSPC`
  (containers/bubblewrap#615, ValveSoftware/steam-runtime#637)
* Fix a double-close on error reading from `--args`, `--seccomp` or
  `--add-seccomp-fd` argument (containers/bubblewrap#558)
* Improve memory allocation behaviour
  (containers/bubblewrap#556, containers/bubblewrap#624)
* Silence various compiler warnings (containers/bubblewrap#559)

Resolves: flatpak#2387
Resolves: flatpak#3477
Resolves: flatpak#5255
Signed-off-by: Simon McVittie <smcv@collabora.com>
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

No branches or pull requests

2 participants