974225 - [mozfile] remove() fails with '[Errno 1] Operation not permitted' when trying to update permissions for a symlink to a system folder

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Debug output

This causes an attempted removal of /usr/include/python2.7, but mach exits before completing this.

Comment 1

[(no longer active)]

Caused by 949600 which has been backed out.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually lets morph this bug because that's a problem with mozfile and not mach. It will block the next merge try.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually this is pretty easy to reproduce on Linux:

$ ln -s /usr/share/ to_delete
$ python -c 'import mozfile; mozfile.remove("_delete")'

I will have a patch soon and will also try to find a test for that, so we do not regress again.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Well, actually the permission change call fails because chmod itself fails when calling it directly:

$ chmod 0700 _delete
chmod: changing permissions of ‘_delete’: Operation not permitted

So it looks like you cannot change the permission of a symlink, and we should simply return early in _update_permissions.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

Silly mistakes! So what does this patch change:

* We do no longer call os.chmod() on symlinks as mentioned above. There is nothing it would change because they already have all the needed permissions

* We call os.stat() for each file and dir we check, and not only once for the path passed into remove(). So all the files/dirs should get the right permissions assigned to.

Comment 6

[Andrew Halberstadt [:ahal]]

Comment on attachment 8378293 [details] [diff] [review]
Patch v1

Review of attachment 8378293 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! One optional readability improvement suggested.

::: mozfile/mozfile/mozfile.py
@@ +184,1 @@
>          mode = dir_mode if os.path.isdir(path) else file_mode

This would probably be better as:

if os.path.isdir(path):
    mode = ...
else:
    mode = ...

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8378293 [details] [diff] [review]
Patch v1

Review of attachment 8378293 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozfile/mozfile/mozfile.py
@@ +184,1 @@
>          mode = dir_mode if os.path.isdir(path) else file_mode

There's a lot of system calls here! We need to keep the system calls in check because they can degrade performance on Windows.

e.g. the |os.path.isdir(path)| is equivalent to |stat.S_ISDIR(path_stats.st_mode)| but the latter doesn't perform a system call.

I'd file a follow-up to optimize this code for common case (no permissions adjustment needed - just attempt delete, catch exception and adjust permissions and retry) and minimizes the number of system calls.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrew Halberstadt [:ahal] from comment #6)
> This would probably be better as:
> 
> if os.path.isdir(path):
>     mode = ...
> else:
>     mode = ...

Yes, that was what I mentioned on IRC. I will do that.

(In reply to Gregory Szorc [:gps] from comment #7)
> There's a lot of system calls here! We need to keep the system calls in
> check because they can degrade performance on Windows.
> 
> e.g. the |os.path.isdir(path)| is equivalent to
> |stat.S_ISDIR(path_stats.st_mode)| but the latter doesn't perform a system
> call.
> 
> I'd file a follow-up to optimize this code for common case (no permissions
> adjustment needed - just attempt delete, catch exception and adjust
> permissions and retry) and minimizes the number of system calls.

Very good point. I will get this addressed. There shouldn't be a need for a follow-up bug. It's an easy change.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #8)
> > I'd file a follow-up to optimize this code for common case (no permissions
> > adjustment needed - just attempt delete, catch exception and adjust
> > permissions and retry) and minimizes the number of system calls.
> 
> Very good point. I will get this addressed. There shouldn't be a need for a
> follow-up bug. It's an easy change.

Looks like I have some trouble with stat.S_ISLNK() and that it doesn't really detect a symlink. So I would say, lets get this really addressed in a follow-up bug.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2

Updated patch, and with 3 new tests included now.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It looks like that os.geteuid() is not available on Windows. So I have to find another way to skip the sympath test for root users.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v2.1

Fixed the skipIf() issue with a single call. All runs fine across platforms.

Comment 13

[Andrew Halberstadt [:ahal]]

Comment on attachment 8378447 [details] [diff] [review]
Patch v2.1

Review of attachment 8378447 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: mozfile/tests/test_remove.py
@@ +137,5 @@
>          mozfile.remove(filepath)
>  
>          self.assertFalse(os.path.exists(filepath))
> +
> +    @unittest.skipIf(mozinfo.isWin, "Symlinks are not supported on Windows")

Instead of mozinfo.isWin you could do "not hasattr(os, 'symlink')"

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8378447 [details] [diff] [review]
Patch v2.1

Talked with Andrew on IRC and it should be enough to have a single reviewer here.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://github.com/mozilla/mozbase/commit/f6faa9e45dea1a203e9023cfd2019be3b6ca9783