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

os.makedirs with exist_ok=True raises PermissionError on Windows 7^ #69769

Closed
danpla mannequin opened this issue Nov 8, 2015 · 17 comments
Closed

os.makedirs with exist_ok=True raises PermissionError on Windows 7^ #69769

danpla mannequin opened this issue Nov 8, 2015 · 17 comments
Labels
OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@danpla
Copy link
Mannequin

danpla mannequin commented Nov 8, 2015

BPO 25583
Nosy @pfmoore, @tjguk, @bitdancer, @vadmium, @zware, @zooba, @danpla
Files
  • makedirs-exist.patch
  • makedirs-exist.2.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2015-11-20.03:47:36.514>
    created_at = <Date 2015-11-08.14:34:33.781>
    labels = ['type-bug', 'library', 'OS-windows']
    title = 'os.makedirs with exist_ok=True raises PermissionError on Windows 7^'
    updated_at = <Date 2015-11-20.03:47:36.512>
    user = 'https://github.com/danpla'

    bugs.python.org fields:

    activity = <Date 2015-11-20.03:47:36.512>
    actor = 'martin.panter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-11-20.03:47:36.514>
    closer = 'martin.panter'
    components = ['Library (Lib)', 'Windows']
    creation = <Date 2015-11-08.14:34:33.781>
    creator = 'plakhotich'
    dependencies = []
    files = ['41008', '41036']
    hgrepos = []
    issue_num = 25583
    keywords = ['patch']
    message_count = 17.0
    messages = ['254339', '254341', '254350', '254355', '254356', '254358', '254365', '254377', '254416', '254420', '254423', '254445', '254471', '254629', '254652', '254949', '254950']
    nosy_count = 8.0
    nosy_names = ['paul.moore', 'tim.golden', 'r.david.murray', 'python-dev', 'martin.panter', 'zach.ware', 'steve.dower', 'plakhotich']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25583'
    versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']

    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 8, 2015

    Since Windows 7 (or even Vista), Windows gives permission error(5, ERROR_ACCESS_DENIED
    if you try to create a directory in a drive root with the same name as a drive itself,
    even if you have administrative permissions. This behavior is not mentioned in Microsoft docs.

    Here is an example session (Windows 7, admin):
    d:\>IF EXIST . echo True
    True
    d:\>mkdir .
    Access is denied.
    d:\>mkdir dir
    d:\>cd dir
    d:\dir>mkdir .
    A subdirectory or file . already exists.
    d:\dir>cd ..
    d:\>
    d:\>py -3
    Python 3.4.3 (v3.4.3:9b73f1c3e601, Feb 24 2015, 22:43:06) [MSC v.1600 32 bit (In
    tel)] on win32
    >>> import os
    >>> os.path.isdir('.')
    True
    >>> os.makedirs('.', exist_ok=True)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "C:\Python34\lib\os.py", line 237, in makedirs
        mkdir(name, mode)
    PermissionError: [WinError 5] ...
    >>> try:
    ...   os.mkdir('.')
    ... except OSError as e:
    ...   print(e.errno)
    ...
    13

    This means that if you want to write portable code, you still need to write like in Python 2:
    if not os.path.isdir(path):
    os.makedirs(path)
    Which makes exist_ok useless.

    The actual problem is in this line (Lib/os.py#l243):
    if not exist_ok or e.errno != errno.EEXIST or not path.isdir(name):

    Due the reasons described above, makedirs shouldn't rely on e.errno, so the right code will be:
    if not (exist_ok and path.isdir(name)):

    I think the issue is pretty serious to be backported.

    @danpla danpla mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Nov 8, 2015
    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 8, 2015

    Probably better solution:

    if not (exist_ok and path.isdir(name) and
            e.errno in (errno.EEXIST, errno.EACCES)):
    

    @bitdancer
    Copy link
    Member

    If you are trying to create a directory named '.' your code will not do anything useful, you might as well skip the call. What's the use case?

    That said, the fix looks reasonable.

    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 8, 2015

    Of course in examples I create '.' by hand, but in real code such things are
    mostly automatic. Assume, for example, that you have a function for downloading
    files:

    def download_file(url, save_as):
      ...
      # Before saving, you must ensure that path exists:
      os.makedirs(os.path.dirname(save_as), exist_ok=True)
      ...

    os.path.abspath is not a solution, because, as it was mentioned, mkdir with
    adrive name gives the same result:
    d:\>mkdir d:\\
    Access is denied.

    Anyway, skipping the calls must be a job of exist_ok=True, otherwise it has
    any sense.

    By the way, do not pay attention to my second message(msg254341): as I said
    at first, mkdirs shouldn't use e.errno at all. Because, as said in docstring,
    makedirs must first check whether a folder exists, and only then call mkdir.
    But currently it works in reverse order, which, in essence, is the source
    of the problem.

    I don't know why it was done that way, because if you try "timeit"
    "try->mkdir->except" vs "if not isdir->mkdir", you will see that the second
    is much faster (x3 (!) times on Windows, x0.7 times on Linux (on ext4 partition)
    on my machine).

    So the best solution is the most straightforward - to replace try-except block with:
    if not path.isdir(name):
    mkdir(name, mode)

    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 8, 2015

    I meant x1.3 times faster on Linux :)

    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 8, 2015

    Of course, exist_ok must be taken into account:
    if not (exist_ok and path.isdir(name)):
    mkdir(name, mode)

    @vadmium
    Copy link
    Member

    vadmium commented Nov 9, 2015

    Daniel: your latest suggestions look like they introduce a race condition. What happens if another thread or process, perhaps also calling makedirs(), creates the directory just after isdir() says it doesn’t exist? Similar to bpo-1608579.

    Perhaps the existing code comment needs to clarify that the exception handling is for a real race condition, not just an excuse to “be happy” :)

    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 9, 2015

    Maybe the solution is to leave OSError catching after the conditional
    (probably with some additional comments):

       if not (exist_ok and path.isdir(name)):
          try:
              mkdir(name, mode)
          except OSError as e:
              if not exist_ok or e.errno != errno.EEXIST or not path.isdir(name):
                  raise

    This should solve the problem. It also gives more or less guarantee
    that errno will be EEXIST, while the current code will also raise an exception
    on EROFS (read-only FS), ENOSPC (no space left) in addition to EACCES on Windows
    (and possibly some other values on various systems - who knows?).

    @vadmium
    Copy link
    Member

    vadmium commented Nov 9, 2015

    Yes that looks like an improvement, though I wonder what’s wrong with your original proposal (performance maybe?). In any case, it definitely needs a comment explaining the first isdir() avoids competing failures that mask EEXIST, and the exception handling avoids the race to create the directory.

    A test case for the test suite would also be good. I understand it should be easy to do for Windows, just make a directory with an absolute path including a drive root like d:\.

    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 9, 2015

    You mean msg254341? As I mentioned recently, it still will raise an exception
    in case of EROFS, ENOSPC and possibly other values, which, as in the case
    with Windows, can be quite unexpected depending on platform and circumstances.

    Of course there is no practical sense to continue when, for example, FS is
    read-only (EROFS), but I think makedirs must be predictable: it should
    first check and only then try to create, as it stated in the docstring.
    When exist_ok=True and directory really exists, a user doesn't expect any
    exceptions from the internally used mkdir, because it simply shouldn't be
    called in this case.

    By the way, why 3.2 and 3.3 were removed from the list? exist_ok was introduced in 3.2.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 10, 2015

    I don’t think we patch 3.2 or 3.3 any more unless it is a security concern. That is why I removed them. I understand 3.4 is due for its last non-security release in a couple weeks.

    I was actually referring to your original suggestion in <https://bugs.python.org/issue25583#msg254339\>, dressed up below:

    except OSError:
        # Cannot rely on checking for EEXIST, since the operating system could give priority to other errors like EACCES or EROFS
        if not (exist_ok and path.isdir(name)):
            raise

    There may be practical reasons to continue if a parent directory exists on a read-only FS. Some OSes can mount writable FSes inside read-only FSes. See <https://marc.info/?l=coreutils-bug&m=124770585425870&w=2\> involving Cygwin, and <https://marc.info/?l=linux-kernel&m=120998905229849&w=2\> involving a Linux regression.

    Anyway, I think I am happy with either your last fix or the first, with an appropriate comment, and hopefully also a test case.

    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 10, 2015

    Yes, it's probably a better solution. If had been more careful, I wouldn't have scribbled so much text here :) .

    But is there any sense in adding Windows-specific test with EACCES since the problem with errno may affect other platforms as well (at least in theory)? It's not a Windows-only issue.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 11, 2015

    It is good to add a regression test for any bug if it’s not too hard. Yes this is not a Windows-only issue, but I understand it is much simpler to produce with Windows. Otherwise you need a special file system setup and a more obscure OS.

    Please review my patch. It would be good to confirm that the test fails on Windows if the fix is not applied. I have only tested this on Linux, and indirectly via Wine.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 13, 2015

    New patch with simpler test case and revised NEWS entry

    @danpla
    Copy link
    Mannequin Author

    danpla mannequin commented Nov 14, 2015

    Looks good to me.
    Without the fix, the test fails on Windows 7 as expected.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 20, 2015

    New changeset 05d6ddf2b7c2 by Martin Panter in branch '3.4':
    Issue bpo-25583: Avoid incorrect errors raised by os.makedirs(exist_ok=True)
    https://hg.python.org/cpython/rev/05d6ddf2b7c2

    New changeset 515f76bf1254 by Martin Panter in branch '3.5':
    Issue bpo-25583: Merge makedirs fix from 3.4 into 3.5
    https://hg.python.org/cpython/rev/515f76bf1254

    New changeset f0ad5067879b by Martin Panter in branch 'default':
    Issue bpo-25583: Merge makedirs fix from 3.5
    https://hg.python.org/cpython/rev/f0ad5067879b

    New changeset 6ec093f78266 by Martin Panter in branch 'default':
    Issue bpo-25583: Add news to 3.6 section
    https://hg.python.org/cpython/rev/6ec093f78266

    @vadmium
    Copy link
    Member

    vadmium commented Nov 20, 2015

    None of the Windows buildbots are failing my particular test. There are other failures, but they look unrelated, so I am calling this fixed.

    @vadmium vadmium closed this as completed Nov 20, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants