Fix broken symlinks install when using install_umask or install_mode #4030

3v1n0 · 2018-08-16T02:36:29Z

Other fixes for #3914 to ignore setting the ownership and mode for broken symlinks after install.

Unfortunately the best way to do this was to cause some stdlib code duplication, because we need to be able to use os.chown with follow_symlinks set to False, but shutil.chown does not support this.

In fact, in general I think we should just never to try to set the ownership of a linked file, as this might be pointing outside the install path (while it's correct to set it only to the link itself), and probably at the same time the same should be for the file-mode of linked files.

3v1n0 · 2018-08-16T02:37:14Z

mesonbuild/minstall.py

@@ -89,6 +89,44 @@ def sanitize_permissions(path, umask):
    except PermissionError as e:
        msg = '{!r}: Unable to set permissions {!r}: {}, ignoring...'
        print(msg.format(path, new_perms, e.strerror))
+    except FileNotFoundError as e:


As said, maybe never do this for symlinks at all?

We could set follow_symlinks=False on os.chmod?

Ah, right I can do that too... the idea here was i wasn't aware of symlinks having different permissions too, but so it seems possible in some platforms, so let's go with that indeed.

Ops, not... So, NotImplementedError: chmod: follow_symlinks unavailable on this platform will happen, thus we should ignore that too.

Yes, looks like it makes more sense to ignore os.chmod/chown completely for symlinks.

3v1n0 · 2018-08-16T02:38:29Z

mesonbuild/minstall.py

@@ -120,6 +158,12 @@ def set_mode(path, mode, default_umask):
        except PermissionError as e:
            msg = '{!r}: Unable to set permissions {!r}: {}, ignoring...'
            print(msg.format(path, mode.perms_s, e.strerror))
+        except FileNotFoundError as e:
+            if os.path.islink(path):


And again here... Probably it's just better not to even try.

3v1n0 · 2018-08-16T02:40:40Z

mesonbuild/minstall.py

@@ -98,7 +136,7 @@ def set_mode(path, mode, default_umask):
    # No chown() on Windows, and must set one of owner/group
    if not is_windows() and (mode.owner or mode.group) is not None:
        try:
-            shutil.chown(path, mode.owner, mode.group)
+            set_chown(path, mode.owner, mode.group, follow_symlinks=False)


Probaby here we could have follow_symlinks=os.file.isfile(path), however I think it doesn't really make much sense (not to say is wrong) to apply on the linked file.

nirbheek

LGTM, plus changing the two places where you point out we should pass os.chmod(..., follow_symlinks=False).

nirbheek · 2018-08-16T12:50:57Z

mesonbuild/minstall.py

@@ -89,6 +89,44 @@ def sanitize_permissions(path, umask):
    except PermissionError as e:
        msg = '{!r}: Unable to set permissions {!r}: {}, ignoring...'
        print(msg.format(path, new_perms, e.strerror))
+    except FileNotFoundError as e:


We could set follow_symlinks=False on os.chmod?

nirbheek · 2018-08-16T12:53:36Z

mesonbuild/minstall.py

@@ -120,6 +158,12 @@ def set_mode(path, mode, default_umask):
        except PermissionError as e:
            msg = '{!r}: Unable to set permissions {!r}: {}, ignoring...'
            print(msg.format(path, mode.perms_s, e.strerror))
+        except FileNotFoundError as e:
+            if os.path.islink(path):


nirbheek · 2018-08-16T12:56:13Z

run_unittests.py

+        Test that installation of broken symlinks works fine.
+        https://github.com/mesonbuild/meson/issues/3914
+        '''
+        testdir = os.path.join(self.common_test_dir, '199 install_mode')


There's no point adding two separate test cases for this, it only slows down the CI. Please replace 66 install subdir above with 199 install_mode and that should cover both.

Ok, I was thinking the same, but at the same time they're eventually following two different codepaths when it comes to call set_mode or sanitize_permissions so I'd prefer to keep them both, but if you're fine with one only, that's ok.

You're right, we should run both. But then let's factor out the test itself to not duplicate it?

Ok, I've done that... Adding a function to perform them both. A class just for that was maybe too much.

nirbheek · 2018-08-16T13:01:00Z

mesonbuild/minstall.py

+        if _group is None:
+            raise LookupError("no such group: {!r}".format(group))
+
+    os.chown(path, _user, _group, dir_fd=dir_fd, follow_symlinks=follow_symlinks)


I noticed that this was taken from CPython and frankly I am shocked that this trivial change doesn't already exist upstream. Fancy opening an upstream bug with a patch so we can eventually get rid of this? :)

Yes, I didn't want to risk to change the behavior at all, so I just reused that.

Actually there is, and I wanted to re-propose the PR already.

Copying code verbatim from Python (or anywhere else for that matter) is ABSOLUTELY FORBIDDEN. At least it is a license violation and at worst it is illegal (assuming the licenses are not compatible, I don't know for sure and am not a lawyer). This will not be accepted UNDER ANY CIRCUMSTANCES!

At least it is a license violation

For some reason I was under the impression that we're licensed under the same license as CPython.

@jpakkane well, ok, I can rephrase that code.... Won't change much though, nor I though it was a license violation here.

jpakkane · 2018-08-16T20:03:07Z

I don't understand under which circumstances this would make sense? Symlinks always have all status bits set (at least that is what ls tells me). Would it not work to check if the thing whose attributes we are trying to change is a symlink, and if it is, simply not do anything?

nirbheek · 2018-08-16T20:08:30Z

I don't understand under which circumstances this would make sense?

FreeBSD seems to allow symlinks to have permissions set, but that's the only operating system. We could revisit this if that turns out to be a useful edge-case. So you're right, we could just ignore os.chmod/chown entirely if os.path.islink.

jpakkane · 2018-08-16T20:19:00Z

Apparently OSX does as well but you can only change them on the command line if the symlink points to nonexisting files (i.e. is broken). 🤦‍♂️

nirbheek · 2018-08-16T20:29:12Z

Apparently OSX does as well but you can only change them on the command line if the symlink points to nonexisting files (i.e. is broken). 🤦‍♂️

The reason why I thought it's fine to not support this right now is because this is for install_subdir which is static files, and I would imagine that any perms on those would have to work on all platforms. I can't imagine someone installing a static broken symlink that requires a certain permission, only on macOS and FreeBSD. 😄

We can, of course, add it if someone asks for it, but seems like needless complexity otherwise (especially if we can't "just" copy CPython code and change it)

3v1n0 · 2018-08-16T23:30:39Z

@jpakkane as I wrote in the first comments of the PR ideally I would have just skipped setting the permissions on the link themselves, then going deeper I noticed that in some platforms this something possible, thus it made sense to apply the permissions to the link itself, but not to the linked target.

Of course we can go back to the simpler solution, but in general I think the way is done now should be quite conform to what is expected for the platform.

EDIT: I've changed it to use another approach instead of using copying the shutil code.

However, as said, this is not just about broken symlinks, what is fixed here is also the case where a symlink points to another position that is not managed by the installer, as in the current state it will try to change the mode of that file too, while this is wrong.

Add test to verify the installation of broken symlinks when a default_umask is set. Reusing the same code of other test, thus sharing the actual test code in a single function.

It's only supported by few platforms when the linked file exists, while it would cause an error otherwise. In any case just implement this via an helper set_chmod function that will handle the case where follow_symlinks is not supported by the platform and will just not set any mod for the link itself (as it would otherwise apply to the linked file). Fixes mesonbuild#3914

Since we're supposed to call this for each installed path, we only should go through what we've installed and not what this point to, as it might be outside our scope or not existent. To do this, since shutil.chown doesn't expose the follow_symlink that os.chown has, we can temporarily replace os.chown with a lambda that acutually passes all the values as we want them, and then restore it to the built-in functions. Not the nicest way, but fixes the issue without having to reimplement what shutil does. Fixes mesonbuild#3914

This could happen when setting an default install mode but with broken symlinks. Fixes mesonbuild#3914

nirbheek

All changes LGTM, needs @jpakkane approval.

3v1n0 commented Aug 16, 2018

View reviewed changes

nirbheek added this to the 0.47.2 milestone Aug 16, 2018

nirbheek self-assigned this Aug 16, 2018

nirbheek reviewed Aug 16, 2018

View reviewed changes

3v1n0 force-pushed the broken-symlink-install-with-mode-fix branch 2 times, most recently from e3598e7 to f8cd612 Compare August 16, 2018 16:46

3v1n0 force-pushed the broken-symlink-install-with-mode-fix branch 6 times, most recently from b0fa3fc to 76dbe4b Compare August 17, 2018 02:52

3v1n0 changed the title ~~Fix broken symlinks install when using install_umask~~ Fix broken symlinks install when using install_umask or install_mode Aug 17, 2018

3v1n0 added 4 commits August 17, 2018 16:41

unittests: test_install_subdir_symlinks_with_default_umask

9f87d5d

Add test to verify the installation of broken symlinks when a default_umask is set. Reusing the same code of other test, thus sharing the actual test code in a single function.

minstall: use follow_symlinks to check executable

abf65c9

This could happen when setting an default install mode but with broken symlinks. Fixes mesonbuild#3914

3v1n0 force-pushed the broken-symlink-install-with-mode-fix branch from 76dbe4b to abf65c9 Compare August 17, 2018 14:41

nirbheek approved these changes Aug 17, 2018

View reviewed changes

jpakkane merged commit 15243e0 into mesonbuild:master Aug 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken symlinks install when using install_umask or install_mode #4030

Fix broken symlinks install when using install_umask or install_mode #4030

3v1n0 commented Aug 16, 2018 •

edited

3v1n0 Aug 16, 2018

nirbheek Aug 16, 2018

3v1n0 Aug 16, 2018

3v1n0 Aug 16, 2018

nirbheek Aug 16, 2018

3v1n0 Aug 16, 2018

nirbheek Aug 16, 2018

3v1n0 Aug 16, 2018

nirbheek left a comment

nirbheek Aug 16, 2018

nirbheek Aug 16, 2018

nirbheek Aug 16, 2018

3v1n0 Aug 16, 2018

nirbheek Aug 16, 2018

3v1n0 Aug 17, 2018

nirbheek Aug 16, 2018

3v1n0 Aug 16, 2018

jpakkane Aug 16, 2018

nirbheek Aug 16, 2018

3v1n0 Aug 16, 2018

jpakkane commented Aug 16, 2018

nirbheek commented Aug 16, 2018

jpakkane commented Aug 16, 2018

nirbheek commented Aug 16, 2018

3v1n0 commented Aug 16, 2018 •

edited

nirbheek left a comment

Fix broken symlinks install when using install_umask or install_mode #4030

Fix broken symlinks install when using install_umask or install_mode #4030

Conversation

3v1n0 commented Aug 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nirbheek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpakkane commented Aug 16, 2018

nirbheek commented Aug 16, 2018

jpakkane commented Aug 16, 2018

nirbheek commented Aug 16, 2018

3v1n0 commented Aug 16, 2018 • edited

nirbheek left a comment

Choose a reason for hiding this comment

3v1n0 commented Aug 16, 2018 •

edited

3v1n0 commented Aug 16, 2018 •

edited