bpo-29248: Fix readlink bug os #5577

SSE4 · 2018-02-07T10:07:23Z

a year already has passed since patch was provided. what should be done to make this into python release?

https://bugs.python.org/issue29248

Signed-off-by: SSE4 <tomskside@gmail.com>

the-knights-who-say-ni · 2018-02-07T10:07:26Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

Signed-off-by: SSE4 <tomskside@gmail.com>

gvanrossum

Otherwise looks good.

gvanrossum · 2018-02-07T15:50:50Z

Lib/test/test_os.py

+                            and os.path.exists(r'C:\ProgramData'),
+                            'Test directories not found')
+    def test_29248(self):
+        target = os.readlink(r'C:\Users\All Users')


I would add the explanation from the tracker issue (why we're using this link instead of creating a fresh one to test) here as a comment.

berkerpeksag · 2018-02-07T17:15:06Z

@SSE4 please let us know when you sign the CLA form so we can merge this PR.

SSE4 · 2018-02-07T17:18:43Z

@berkerpeksag I already did earlier today

berkerpeksag · 2018-02-07T17:19:27Z

Misc/NEWS.d/next/Windows/2018-02-07-17-50-48.bpo-29248.Xzwj-6.rst

@@ -0,0 +1 @@
+fix os.readlink() on Windows


Please make the news entry more descriptive and add "Patch by Craig Holmquist and Your Name."

serhiy-storchaka · 2018-02-07T22:37:30Z

Modules/posixmodule.c

@@ -7440,7 +7440,7 @@ win_readlink(PyObject *self, PyObject *args, PyObject *kwargs)
        return NULL;
    }
    print_name = rdb->SymbolicLinkReparseBuffer.PathBuffer +
-                 rdb->SymbolicLinkReparseBuffer.PrintNameOffset;
+                 (rdb->SymbolicLinkReparseBuffer.PrintNameOffset / 2);


Can PrintNameOffset be odd?

PathBuffer is of type wchar_t. Strictly this should use sizeof(wchar_t) instead of 2. Alternatively it can use two casts to have the compiler do the work: (wchar_t *)((char *)rdb->SymbolicLinkReparseBuffer.PathBuffer + rdb->SymbolicLinkReparseBuffer.PrintNameOffset).

eryksun · 2018-02-08T01:17:15Z

Modules/posixmodule.c

@@ -7440,7 +7440,7 @@ win_readlink(PyObject *self, PyObject *args, PyObject *kwargs)
        return NULL;
    }
    print_name = rdb->SymbolicLinkReparseBuffer.PathBuffer +
-                 rdb->SymbolicLinkReparseBuffer.PrintNameOffset;
+                 (rdb->SymbolicLinkReparseBuffer.PrintNameOffset / 2);

    result = PyUnicode_FromWideChar(print_name,
                    rdb->SymbolicLinkReparseBuffer.PrintNameLength/2);


Strictly this should also use sizeof(wchar_t) instead of hard coding 2.

… work

SSE4 · 2018-02-09T06:34:00Z

/cc @DinoV @zooba can please take a look at this issue related to Microsoft Windows?

berkerpeksag

All review comments have been addressed and this looks good to me. I will wait for a day or two to give everyone a chance to look at the patch and merge it. Thanks!

SSE4 · 2018-02-12T15:25:54Z

@berkerpeksag okay, 3 days passed already, what's the next action to be done?

miss-islington · 2018-02-12T17:10:37Z

Thanks @SSE4 for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-02-12T17:12:20Z

GH-5640 is a backport of this pull request to the 3.6 branch.

The PrintNameOffset field of the reparse data buffer was treated as a number of characters instead of bytes. (cherry picked from commit 3c34aad) Co-authored-by: SSE4 <tomskside@gmail.com>

berkerpeksag · 2018-02-12T20:43:59Z

@Mariatta do I need to backport this to 3.7 branch manually or is there some sort of queue?

miss-islington · 2018-02-12T21:00:57Z

Thanks @SSE4 for the PR, and @berkerpeksag for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-02-12T21:02:10Z

GH-5644 is a backport of this pull request to the 3.7 branch.

The PrintNameOffset field of the reparse data buffer was treated as a number of characters instead of bytes. (cherry picked from commit 3c34aad) Co-authored-by: SSE4 <tomskside@gmail.com>

Mariatta · 2018-02-12T21:02:34Z

@berkerpeksag I think this was merged at the same time miss-islington was being deployed, so it missed that webhook event. I've reapplied the label to trigger the backport.

berkerpeksag · 2018-02-12T21:05:43Z

@Mariatta thanks!

The PrintNameOffset field of the reparse data buffer was treated as a number of characters instead of bytes. (cherry picked from commit 3c34aad) Co-authored-by: SSE4 <tomskside@gmail.com>

merwok · 2018-02-18T00:24:43Z

@berkerpeksag I noticed that the news file is the only one in the repo to use CRLF.

bpo-29248: Fix readlink bug os

2cb8ba6

Signed-off-by: SSE4 <tomskside@gmail.com>

the-knights-who-say-ni added the CLA not signed label Feb 7, 2018

bedevere-bot added the awaiting review label Feb 7, 2018

add Misc/NEWS.d entry

3c8ce39

Signed-off-by: SSE4 <tomskside@gmail.com>

gvanrossum reviewed Feb 7, 2018

View reviewed changes

add explanation from the tracker issue

9893c55

Cosmetic fixes

d23bd99

berkerpeksag reviewed Feb 7, 2018

View reviewed changes

make the news entry more descriptive

b0e82eb

serhiy-storchaka reviewed Feb 7, 2018

View reviewed changes

eryksun reviewed Feb 8, 2018

View reviewed changes

SSE4 added 2 commits February 8, 2018 09:48

use sizeof(wchar_t) instead of hard coding 2

a816bcc

PrintNameOffset can be odd, use two casts to have the compiler do the…

3234a71

… work

berkerpeksag removed the CLA not signed label Feb 9, 2018

the-knights-who-say-ni added the CLA signed label Feb 9, 2018

use reST format in NEWS entry

0d66fb3

berkerpeksag approved these changes Feb 9, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Feb 9, 2018

berkerpeksag added needs backport to 3.6 labels Feb 12, 2018

berkerpeksag merged commit 3c34aad into python:master Feb 12, 2018

bedevere-bot removed the awaiting merge label Feb 12, 2018

bedevere-bot removed the needs backport to 3.6 label Feb 12, 2018

Mariatta added needs backport to 3.7 and removed needs backport to 3.7 labels Feb 12, 2018

bedevere-bot removed the needs backport to 3.7 label Feb 12, 2018

SSE4 mentioned this pull request Feb 13, 2018

conan export fails in Windows Docker container conan-io/conan#2439

Closed

3 tasks

merwok changed the title ~~bpo-29248: Fix readlink bug os~~ s/merwokbpo-29248: Fix readlink bug os Feb 18, 2018

merwok changed the title ~~s/merwokbpo-29248: Fix readlink bug os~~ bpo-29248: Fix readlink bug os Feb 18, 2018

Uh oh!

bpo-29248: Fix readlink bug os #5577

bpo-29248: Fix readlink bug os #5577

Uh oh!

Conversation

SSE4 commented Feb 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Feb 7, 2018

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

berkerpeksag commented Feb 7, 2018

Uh oh!

SSE4 commented Feb 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eryksun Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SSE4 commented Feb 9, 2018

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

SSE4 commented Feb 12, 2018

Uh oh!

miss-islington commented Feb 12, 2018

Uh oh!

bedevere-bot commented Feb 12, 2018

Uh oh!

berkerpeksag commented Feb 12, 2018

Uh oh!

miss-islington commented Feb 12, 2018

Uh oh!

bedevere-bot commented Feb 12, 2018

Uh oh!

Mariatta commented Feb 12, 2018

Uh oh!

berkerpeksag commented Feb 12, 2018

Uh oh!

merwok commented Feb 18, 2018

Uh oh!

Uh oh!

SSE4 commented Feb 7, 2018 •

edited

Loading

eryksun Feb 8, 2018 •

edited

Loading