950227 - [mozfile] mozfile.remove() should always set write permissions on files and folders before trying to remove them

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

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

As Andrew pointed out on bug 949600 we have a couple of failures in removing files created by the tests of mozinstall. 

https://tbpl.mozilla.org/php/getParsedLog.php?id=31893804&tree=Try

WindowsError: [Error 5] Access is denied: 'c:\\users\\cltbld\\appdata\\local\\temp\\tmp5_gght\\zip\\firefox\\firefox.exe'

I digged into this problem and found the culprit. As it looks like we are modifying the file attributes in some strange ways that the file cannot be removed anymore from the disk by mozfile.remove():

https://github.com/mozilla/mozbase/blob/master/mozfile/mozfile/mozfile.py#L75

        mode = bundle.getinfo(name).external_attr >> 16 & 0x1FF
        os.chmod(filename, mode)

This specific code went in the tree by Andrews patch on bug 864939.

Comment 1

[Andrew Halberstadt [:ahal]]

That patch should just be preserving the file mode to be the same as it was when it was bundled. As noted on bug 864939, we can only set the read bit using chmod on Windows. So if backing out that patch fixes the issue, then maybe the solution is just wrapping those lines in an 'if !isWin' block.

Comment 2

[Andrew Halberstadt [:ahal]]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=d9919448a8bd

I think an alternative is to just modify the tests to use mozfile.remove instead. One thing I don't understand however, is that bug 864939 was landed before this sync, and the sync also didn't touch any of the mozinstall tests. So I'm not sure how 864939 could be the culprit. I guess we'll find out soon enough :)

Comment 3

[Andrew Halberstadt [:ahal]]

Forgot to import mozinfo in last try run:
https://tbpl.mozilla.org/?tree=Try&rev=e8bc3f99e28d

Comment 4

[Andrew Halberstadt [:ahal]]

Weird, mozinfo import error while running mozbase tests? Oh well, I just used platform instead. Third time's the charm:
https://tbpl.mozilla.org/?tree=Try&rev=64353cac094a

Comment 5

[Andrew Halberstadt [:ahal]]

That seemed to fix it. I guess this line was the one affecting the mozinstall tests: https://hg.mozilla.org/try/rev/64353cac094a#l31.339

Comment 6

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

Andrew, what have you changed in the last push? It's green.

Comment 7

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - don't try to preserve file mode on windows

Since os.chmod doesn't work on windows, just skip the file mode preservation if on windows. I didn't use mozinfo because there was an import error when I did for some reason, also it seems like overkill to add a dependency on it just for isWin.

Comment 8

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

Comment on attachment 8348233 [details] [diff] [review]
Patch 1.0 - don't try to preserve file mode on windows

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

::: mozfile/mozfile/mozfile.py
@@ +76,5 @@
> +
> +        # os.chmod can only set read bit on Windows, so don't bother
> +        if not platform.system() in ['Microsoft', 'Windows']:
> +            mode = bundle.getinfo(name).external_attr >> 16 & 0x1FF
> +            os.chmod(filename, mode)

I wasn't really sure about this one, so I digged around more about this and found the causing problem. It's not the code here but how the firefox.exe file has been packaged in the zip archive. Please see:

>>> bundle.getinfo('firefox/firefox.exe').external_attr
32
>>> bundle.getinfo('firefox/firefox.exe').external_attr >> 16
0

So we are revoking each permission on that file which will cause us to fail on Windows.

From my point of view we have to fix that zip file, but also update mozfile.remove() so that we re-introduce setting the write permissions, which was done before my last patch. Only that way we can be sure that we can always remove files and folders we have created ourselves.

Comment 9

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

So it's indeed an issue which has been revealed by the code removal in my patch on bug 944684.

Comment 10

[Andrew Halberstadt [:ahal]]

I'm not sure I have a firm grasp of the problem, Henrik would you mind writing a patch or pointing me towards the changes I need to make?

Comment 11

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

Yes, I will work on it tomorrow.

Comment 12

[Jarek Śmiejczak [:jotes]]

Attachment: 0001-Bug-950277-restored-recursive-remove-of-directory-co.patch

:whimboo
I'm not sure but after reading previous version of remove, you've removed cleaning contents of directory before total removal.
There's a problem if directory isn't empty and file inside don't have right perms (correct me if i'm wrong).

My patch passes all tests and fixes issues connected with mozinstall tests (everything is removed properly).

Comment 13

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

Comment on attachment 8359484 [details] [diff] [review]
0001-Bug-950277-restored-recursive-remove-of-directory-co.patch

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

This patch is not complete and would need also have to test items in this folder and subfolders. Further tests would have to be added. I will try to get to a patch by today.

Comment 14

[Jarek Śmiejczak [:jotes]]

Attachment: 0001-Bug-950277-restored-recursive-remove-of-directory-co.patch

:whimboo

I've extended my patch a little :) according to our discussions on irc. I hope it's finally is on valid path.

Comment 15

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

Comment on attachment 8360141 [details] [diff] [review]
0001-Bug-950277-restored-recursive-remove-of-directory-co.patch

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

We are getting closer. But there are still some things which are not that clear to me and need an update. Please see my inline comments.

::: mozfile/mozfile/mozfile.py
@@ +143,5 @@
> +
> +    :param path: path of directory/file of which modes must be changed
> +    """
> +    mode = os.stat(path)[stat.ST_MODE]
> +    os.chmod(path, mode & ~stat.S_IWUSR & ~stat.S_IWGRP & ~stat.S_IWOTH)

I don't think that we put the code into those extra methods. That's really only necessary when we want to remove a file or a complete folder. So please check how the code was behaving before my patch on bug 944684, and re-add those lines with an updated comment so it doesn't mention that it only applies to Windows NT.

@@ +150,3 @@
>  ### utilities for removal of files and directories
>  
>  def rmtree(dir):

I liked the idea from you to mark this method as deprecated. Would you mind doing that?

@@ +197,4 @@
>          return
>  
>      path_stats = os.stat(path)
> +    file_remove_mode = path_stats.st_mode | stat.S_IRUSR | stat.S_IWUSR

I would remove the `remove` part from the variable.

@@ +198,5 @@
>  
>      path_stats = os.stat(path)
> +    file_remove_mode = path_stats.st_mode | stat.S_IRUSR | stat.S_IWUSR
> +    dir_remove_mode = path_stats.st_mode | stat.S_IRUSR | stat.S_IWUSR
> +    dir_remove_mode |= stat.S_IXUSR

You can do `dir_mode |= stat.S_IXUSR` without having to add two identical lines above.

@@ +202,5 @@
> +    dir_remove_mode |= stat.S_IXUSR
> +
> +    # Sets speciifed pemissions depending on filetype.
> +    smart_chmod = lambda path: os.chmod(path, dir_remove_mode\
> +                        if os.path.isdir(path) else file_remove_mode)

lambda code is nice but also hard to follow. I would simply define a closure in that method.

::: mozfile/tests/test_rmtree.py
@@ +59,5 @@
>  
>          # Check deletion is successful
>          self.assertFalse(os.path.exists(self.tempdir))
> +
> +    def test_remove_readonly_directory(self):

We would need something similar for a simple read-only file.

@@ +70,5 @@
> +        # Checking if shutils.rmtree because of lack of write permissions.
> +        if mozinfo.isWin:
> +            self.assertRaises(WindowsError, shutil.rmtree, dirpath)
> +        else:
> +            self.assertRaises(OSError, shutil.rmtree, dirpath)

Sorry, but I don't got those lines. Why are they needed? If it is just for checking if files are read-only, we should leave it out. I think we can trust the OS by setting the correct permissions.

@@ +73,5 @@
> +        else:
> +            self.assertRaises(OSError, shutil.rmtree, dirpath)
> +
> +        # However, mozfile should change write permissions and remove dir.
> +        mozfile.remove(dirpath)

If we fail here and the folder remains as read-only, how would shutil.rmtree() handle that folder in tearDown? I think it will also fail, and the folder will remain.

@@ +77,5 @@
> +        mozfile.remove(dirpath)
> +
> +        self.assertFalse(os.path.exists(dirpath))
> +
> +    def test_remove_nested_readonly_file(self):

I would call this `test_remove_readonly_tree`.

::: mozfile/tests/test_traversal.py
@@ +1,1 @@
> +#!/usr/bin/env python

I don't think that we need this test given my other comment in the mozfile.py.

Comment 16

[Jarek Śmiejczak [:jotes]]

Attachment: 0001-Bug-950277-restored-recursive-remove-of-directory-co.patch

:whimboo
I've updated patch. Please review again.

Comment 17

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

Comment on attachment 8360650 [details] [diff] [review]
0001-Bug-950277-restored-recursive-remove-of-directory-co.patch

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

::: mozfile/mozfile/mozfile.py
@@ +173,5 @@
>      path_stats = os.stat(path)
> +    file_mode = path_stats.st_mode | stat.S_IRUSR | stat.S_IWUSR
> +    dir_mode = file_mode | stat.S_IXUSR
> +
> +    # Sets speciifed pemissions depending on filetype.

nit: spelling mistake in specified.

@@ +174,5 @@
> +    file_mode = path_stats.st_mode | stat.S_IRUSR | stat.S_IWUSR
> +    dir_mode = file_mode | stat.S_IXUSR
> +
> +    # Sets speciifed pemissions depending on filetype.
> +    def smart_chmod(path):

I would call it update_permissions

::: mozfile/tests/test_rmtree.py
@@ +3,1 @@
>  import os

You missed to rename this file.

@@ +16,5 @@
> +def mark_readonly(path):
> +    """Removes all write permissions from given file/directory.
> +
> +    :param path: path of directory/file of which modes must be changed
> +    """

nit: blank line before the ending quotes.

@@ +71,5 @@
>          self.assertFalse(os.path.exists(self.tempdir))
> +
> +    def test_remove_readonly_tree(self):
> +        """ Test that the mozfile.remove is able to remove readonly
> +        directories """

A simple docstring should fit into a single line. Otherwise you have to change the format. And no blank line after the closing quotes.

@@ +73,5 @@
> +    def test_remove_readonly_tree(self):
> +        """ Test that the mozfile.remove is able to remove readonly
> +        directories """
> +
> +        dirpath = os.path.join(self.tempdir, "foo")

can we let the stub create a tree like 'nested_tree'? Just to be sure that no-one changes 'foo' and we get problems here. Or rename foo to nested_tree.

Comment 18

[Jarek Śmiejczak [:jotes]]

Attachment: 0001-Bug-950277-restored-recursive-remove-of-directory-co.patch

:whimboo
Updated :-)

Comment 19

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

Comment on attachment 8360717 [details] [diff] [review]
0001-Bug-950277-restored-recursive-remove-of-directory-co.patch

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

That patch looks fine functionality wise. I will get it pushed to try so we can check that everything works across platforms.

Please see my inline comments for the nits I would like to see fixed. Then you have my r+. Feel free to carry it over then.

::: mozfile/mozfile/mozfile.py
@@ +8,4 @@
>  import os
>  import shutil
>  import stat
> +import time

nit: wrong position. It should be after tempfile.

@@ +129,5 @@
>      :param dir: directory to be removed
>      """
>  
> +    warnings.warn("mozfile.rmtree() is deprecated. Ensure to update your code"
> +        " to use mozfile.remove() directly", DeprecationWarning, stacklevel=2)

I would make the comment shorter: "mozfile.rmtree() is deprecated in favor of mozfile.remove()".

::: mozfile/tests/test_remove.py
@@ +2,5 @@
> +
> +import os
> +import shutil
> +import tempfile
> +import shutil

shutils is contained twice and tempfile should be located after stat.

::: mozfile/tests/test_rmtree.py
@@ -1,1 @@
> -#!/usr/bin/env python

Hm, usually git should show that the file got moved. Have you used 'git mv' to rename the file or did you do a mv on system level?

Comment 20

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

Try server run:
https://tbpl.mozilla.org/?tree=Try&rev=a7df8f81d40a

Comment 21

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

This is actually also blocking our Mozmill 2.0.4 release.

Comment 22

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

The try server run is green in regards of mozbase tests! As I talked with Jarek I will quickly update the minor nits and then push it to master.

Comment 23

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

I have landed the patch with the minor adjustments on master:
https://github.com/mozilla/mozbase/commit/b1c67a16c2a17fe63de8908e5d4f4cf7c6f2189c

We should be fine here now. Thanks Jarek for helping us getting this blocker fixed!