Issue 29248: os.readlink fails on Windows

Messages (9) msg285294 - (view) Author: Eryk Sun (eryksun) * Date: 2017-01-12 08:09

win_readlink in Modules/posixmodule.c mistakenly treats the PrintNameOffset field of the reparse data buffer as a number of characters instead of bytes. Thus, if the offset is non-zero, the value returned is incorrect stack garbage. For example, the following should return "C:\\ProgramData":

    >>> os.readlink(r'C:\Users\All Users')
    '\u6c20\u3012\u041f\x01\u2768\u60b2\u031b\x02\x05\x00\u031e\x06\u8c01\u4012'

Craig Holmquist found this bug, as detailed in message 277385. He included a fix in the patch for issue 23407, but this should be addressed in the next maintenance release of 3.5 and 3.6.

msg285501 - (view) Author: Craig Holmquist (craigh) Date: 2017-01-14 20:38

New patch with test.

I'm not sure if C:\Users\All Users and C:\ProgramData exist with those names on non-English installations of Windows.  I set the test to skip if they aren't found.

msg286466 - (view) Author: Berker Peksag (berker.peksag) * Date: 2017-01-30 02:20

Thanks for the patch. I think it would be better to use the existing framework in Win32SymlinkTests to create a test file instead of using a hardcoded path like 'C:\Users\All Users' (e.g. use Win32SymlinkTests.filelink and Win32SymlinkTests.filelink_target to create a link)

msg286469 - (view) Author: Eryk Sun (eryksun) * Date: 2017-01-30 03:29

os.symlink calls CreateSymbolicLink, which creates the reparse data buffer with the print name stored first, so the offset is always 0. Otherwise we would have noticed this problem already. For example:

    >>> os.symlink('C:\\', 'link', True)
    >>> os.system('fsutil reparsepoint query link')
    Reparse Tag Value : 0xa000000c
    Tag value: Microsoft
    Tag value: Name Surrogate
    Tag value: Symbolic Link

    Reparse Data Length: 0x00000020
    Reparse Data:
    0000:  06 00 0e 00 00 00 06 00  00 00 00 00 43 00 3a 00  ............C.:.
    0010:  5c 00 5c 00 3f 00 3f 00  5c 00 43 00 3a 00 5c 00  \.\.?.?.\.C.:.\.

As you can see above, CreateSymbolicLink stores the "PrintName" DOS path (e.g. "C:\") first, with an offset of 0, followed by the "SubstituteName" NT path (e.g. "\??\C:\").

The "All Users" link, on the other hand, seems to have been created manually with an inverted order. I have ctypes code to manually create a similar link (calling OpenProcessToken/AdjustTokenPrivileges to enable the required privilege and CreateFile/DeviceIoControl to set the reparse point), but I doubt it's worth adding it just to test this bug.

msg311781 - (view) Author: SSE4 (SSE4) * Date: 2018-02-07 10:16

opened PR https://github.com/python/cpython/pull/5577

msg312063 - (view) Author: Berker Peksag (berker.peksag) * Date: 2018-02-12 17:10

New changeset 3c34aad4e7a95913ec7db8e5e948a8fc69047bf7 by Berker Peksag (SSE4) in branch 'master':
bpo-29248: Fix os.readlink() on Windows (GH-5577)
https://github.com/python/cpython/commit/3c34aad4e7a95913ec7db8e5e948a8fc69047bf7

msg312067 - (view) Author: Berker Peksag (berker.peksag) * Date: 2018-02-12 18:14

New changeset a1d33f742515dc70ae99bc3ea1c851729522afc3 by Berker Peksag (Miss Islington (bot)) in branch '3.6':
bpo-29248: Fix os.readlink() on Windows (GH-5577)
https://github.com/python/cpython/commit/a1d33f742515dc70ae99bc3ea1c851729522afc3

msg312082 - (view) Author: miss-islington (miss-islington) Date: 2018-02-12 21:39

New changeset 74ebbaeb566dc10031756430ec5c896e56d0e491 by Miss Islington (bot) in branch '3.7':
bpo-29248: Fix os.readlink() on Windows (GH-5577)
https://github.com/python/cpython/commit/74ebbaeb566dc10031756430ec5c896e56d0e491

msg312088 - (view) Author: Berker Peksag (berker.peksag) * Date: 2018-02-12 22:47

Thank you, Craig and SSE4.

History Date User Action Args 2022-04-11 14:58:41adminsetgithub: 73434 2018-02-12 22:47:00berker.peksagsetstatus: open -> closed
versions: + Python 3.8
messages: + msg312088

resolution: fixed
stage: patch review -> resolved

2018-02-12 21:39:47miss-islingtonsetnosy: + miss-islington
messages: + msg312082
2018-02-12 21:02:09miss-islingtonsetpull_requests: + pull_request5445 2018-02-12 18:14:10berker.peksagsetmessages: + msg312067 2018-02-12 17:14:15berker.peksagsetversions: - Python 3.5 2018-02-12 17:12:19miss-islingtonsetpull_requests: + pull_request5440 2018-02-12 17:10:43berker.peksagsetmessages: + msg312063 2018-02-07 10:16:16SSE4setnosy: + SSE4
messages: + msg311781
2018-02-07 10:07:24python-devsetpull_requests: + pull_request5395 2017-01-30 03:29:11eryksunsetmessages: + msg286469 2017-01-30 02:20:31berker.peksagsetnosy: + berker.peksag

messages: + msg286466
stage: needs patch -> patch review

2017-01-14 20:38:49craighsetfiles: + issue29248-2.patch
nosy: + craigh
messages: + msg285501 2017-01-14 20:11:02eryksunlinkissue23407 dependencies 2017-01-14 15:31:45craighsetfiles: + issue29248.patch
keywords: + patch 2017-01-12 08:09:20eryksuncreate