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 #577

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Make --symlink idempotent #577

merged 1 commit into from
Jan 31, 2024

Conversation

jkaivo
Copy link
Contributor

@jkaivo jkaivo commented 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.

jkaivo added a commit to jkaivo/bubblewrap that referenced this pull request 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#577

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

Fixes #549

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this! I don't know whether it's OK for this to be unconditional, or whether that would be a breaking change; that can be a separate thread. For now I'm reviewing the implementation as if it's OK to do this simple version.

bubblewrap.c Outdated Show resolved Hide resolved
bubblewrap.c Outdated Show resolved Hide resolved
bubblewrap.c Outdated Show resolved Hide resolved
bubblewrap.c Outdated Show resolved Hide resolved
@smcv
Copy link
Collaborator

smcv commented Apr 22, 2023

@alexlarsson, @cgwalters: Do you think it's OK to make --symlink idempotent unconditionally, or do you think there's a risk of breaking existing bwrap callers that are expecting the usual atomic "if exists then error else create" semantics?

If you think it needs to be explicit, there are a couple of possible spellings.

The simplest but least general would be --symlink-idempotent desired-target /dest, like the relationship between --ro-bind and --bind.

But I think it might be better to do something more like the recently-added --perms, like maybe these (which would not all have to be implemented immediately, but it could be useful to have syntax available for them):

  • --conflict exclusive --symlink t /dest: if /dest already exists then error, even if it's a symlink to t
  • --conflict idempotent --symlink t /dest: if /dest already exists with the desired target then ignore, else error
  • --conflict rename --symlink t /dest: if /dest already exists and doesn't have the desired target, then try to rename it to /dest.old.{unique string}, and if successful, leave it there
  • --conflict rename-or-remove --symlink t /dest: if /dest already exists and doesn't have the desired target, then try to rename it to /dest.old.{unique string}; if successful and empty, remove the old name; if successful and non-empty, keep it

@smcv
Copy link
Collaborator

smcv commented Apr 22, 2023

If we had a --conflict option acting as a modifier for the next action, then we'd be able to expand that to other file types over time (for example --dir currently defaults to the equivalent of --conflict idempotent --dir). Obviously implementing that wouldn't be a blocker for this feature: it would be fine for an initial implementation of --conflict to only work on --symlink.

bubblewrap.c Outdated Show resolved Hide resolved
Copy link

@AndersBlomdell AndersBlomdell left a comment

Choose a reason for hiding this comment

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

A very nice enhancement to flatpak, please merge.

Maybe after fixing the whitespace problem on line 1493 (two extra blanks before break)

@smcv
Copy link
Collaborator

smcv commented Sep 28, 2023

I was really hoping for a review of this from @cgwalters or @alexlarsson, who have a better overview than I do of all the places where bubblewrap is used and the ways in which it needs to stay backwards-compatible.

@jchernand0 approved these changes

@AndersBlomdell approved these changes

Did I miss one of the maintainers of bubblewrap adding you to the project?

Or if you are not maintainers, does this mean we can count on receiving contributions from you if this change is merged but turns out to cause regressions?

bubblewrap.c Outdated
Comment on lines 1480 to 1490
char existing[PATH_MAX + 1] = "";
ssize_t elen = readlink (dest, existing, sizeof (existing) - 1);
if (elen < 0)
{
if (errno == EINVAL)
die ("Can't make symlink at %s: destination exists and is not a symlink", op->dest);
else
die_with_error ("Can't make symlink at %s: destination exists, and cannot read symlink target", op->dest);
}

existing[elen] = '\0';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use readlink_malloc() (into a new cleanup_free char *existing = NULL) in preference to a statically-sized buffer. This returns a non-null string on success, or NULL with errno set on failure.

Copy link

Choose a reason for hiding this comment

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

hey @jkaivo will you be able to do this chages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just seeing this. I don't see what actual advantage readlink_malloc() has over POSIX readlink() with an appropriately sized buffer. Since submitting this patch, I have solved my personal use-case issues by only using Steam on my Steam Deck, so I have very little motivation to make additional changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see what actual advantage readlink_malloc() has over POSIX readlink() with an appropriately sized buffer

If it was possible to choose an appropriately sized buffer, then there would be no advantage. The problem with POSIX readlink() is that there is no correct size for the buffer, because Linux PATH_MAX is a lie (it is entirely valid to have paths longer than PATH_MAX).

@AndersBlomdell
Copy link

I was really hoping for a review of this from @cgwalters or @alexlarsson, who have a better overview than I do of all the places where bubblewrap is used and the ways in which it needs to stay backwards-compatible.

@jchernand0 approved these changes

@AndersBlomdell approved these changes

Did I miss one of the maintainers of bubblewrap adding you to the project?

Or if you are not maintainers, does this mean we can count on receiving contributions from you if this change is merged but turns out to cause regressions?

Not a maintainer, but since this patch actually made flatpak useful for me (I add symlinks below /run/user), I will of course help to the best of my abilities.

@cwajh
Copy link

cwajh commented Oct 19, 2023

@smcv If you need an LGTM from @alexlarsson or @cgwalters before you'll feel comfortable accepting this pull request, what do we have to do to get their attention? The pull request has been waiting for them since April and I'm sure @jkaivo would be more motivated to implement your comment if we had some indication that the patch might actually go in once the fix is made.

If the concern is backwards-incompatibility with current users... I for one can testify that certain users of bwrap are already assuming the functionality is idempotent, and choking when it isn't. That's the reason I found this thread in the first place: a flatpak app I use fails to start if one of the many hidden directories in my homedir has a symlink to somewhere. I'm working around it with mount --bind but it's fairly annoying to need special root jiggery-pokery to do something that should be a simple ln.

Thanks for your help.

@alexlarsson
Copy link
Collaborator

Personally I think it is pretty safe to just add this as-is. I'm not aware of anyone using bwrap --symlink and then specifically looking for some kind of EEXIST error and handling that in a custom way. It would be really hard to extract that particular error anyway as other things could error out.

So, LGTM, but I would prefer if all the changes were squashed into one commit.

@smcv
Copy link
Collaborator

smcv commented Jan 24, 2024

It sounds as though the submitter of this PR is no longer interested in contributing it, so I'll try to find time among my other responsibilities to rebase this and fix my review comment myself.

@AndersBlomdell
Copy link

It sounds as though the submitter of this PR is no longer interested in contributing it, so I'll try to find time among my other responsibilities to rebase this and fix my review comment myself.

Something along these lines?:

diff --git a/bubblewrap.c b/bubblewrap.c
index e894af7..65e5563 100644
--- a/bubblewrap.c
+++ b/bubblewrap.c
@@ -1468,7 +1468,24 @@ setup_newroot (bool unshare_pid,
         case SETUP_MAKE_SYMLINK:
           assert (op->source != NULL);  /* guaranteed by the constructor */
           if (symlink (op->source, dest) != 0)
-            die_with_error ("Can't make symlink at %s", op->dest);
+            {
+              if (errno == EEXIST)
+                {
+                  cleanup_free char *existing = readlink_malloc(dest);
+                  if (existing == NULL) {
+                    if (errno == EINVAL)
+                      die ("Can't make symlink at %s: destination exists and is not a symlink", op->dest);
+                    else
+                      die_with_error ("Can't make symlink at %s: destination exists, and cannot read symlink target", op->dest);
+                  }
+
+                  if (strcmp (existing, op->source) == 0)
+                    break;
+
+                  die ("Can't make symlink at %s: existing destination is %s)", op->dest, existing);
+                }
+              die_with_error ("Can't make symlink at %s", op->dest);
+            }
           break;
 
         case SETUP_SET_HOSTNAME:

@smcv
Copy link
Collaborator

smcv commented Jan 31, 2024

Something along these lines?:

Yes, a lot like that (but with some coding style fixes). Thanks!

Please send pull requests for any future changes that you want to propose: the limiting factor on this project is the maintainers' limited time, and that's more efficiently used if we aren't passing diff output around like it's the 1990s.

+ cleanup_free char *existing = readlink_malloc(dest);

coding style: space before (

+ if (existing == NULL) {

coding style: this project uses GNU-style indentation

+ die ("Can't make symlink at %s: existing destination is %s)", op->dest, existing);

The trailing ) looks wrong here

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.

Resolves: containers#549
Signed-off-by: Jakob Kaivo <jkk@ung.org>
[Anders Blomdell: use readlink_malloc()]
[smcv: Squash multiple changes into one; coding style fixes]
Co-authored-by: Anders Blomdell
Co-authored-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
@smcv
Copy link
Collaborator

smcv commented Jan 31, 2024

I'll merge this after CI succeeds, unless another maintainer objects.

smcv added a commit to smcv/bubblewrap that referenced this pull request Jan 31, 2024
Signed-off-by: Simon McVittie <smcv@collabora.com>
@smcv smcv merged commit 51a8188 into containers:main Jan 31, 2024
5 checks passed
smcv added a commit to smcv/bubblewrap that referenced this pull request Jan 31, 2024
Signed-off-by: Simon McVittie <smcv@collabora.com>
smcv added a commit that referenced this pull request Feb 5, 2024
Add documentation and test coverage for #577
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants