bpo-33687: uu: fix call to os.chmod #7282

timofurrer · 2018-05-31T11:33:46Z

This change addresses the issue reported here: https://bugs.python.org/issue33687

Do I have to pull request this to other branches, too?
The issue exists on all actively maintained cpython branches.

https://bugs.python.org/issue33687

berkerpeksag · 2018-05-31T12:56:18Z

Lib/uu.py

@@ -134,7 +134,7 @@ def decode(in_file, out_file=None, mode=None, quiet=False):
        elif isinstance(out_file, str):
            fp = open(out_file, 'wb')
            try:
-                os.path.chmod(out_file, mode)
+                os.chmod(out_file, mode)
            except AttributeError:


Perhaps I'm missing something but os.chmod() is defined undonditionally in Modules/posixmodule.c:

cpython/Modules/posixmodule.c

Line 2821 in 941ec21

os_chmod_impl(PyObject *module, path_t *path, int mode, int dir_fd,

Do we still need the try...except AttributeError check? It was added in 1995 in 8b74512#diff-bd8ce21226a634ebc2701b92641e3424R108 and I think we can remove it now.

Agree. I've removed the try ... except block.

serhiy-storchaka · 2018-06-03T08:58:30Z

Why test_decodetwice was changed? Seems this breaks the purpose of this test.

It would be better to test with a real os.chmod instead of the mock. Try to create a file with a mode 0o444 and check that the resulting file is read-only. This will expose other error in using os.chmod. You need first call os.chmod with mode | stat.S_IWRITE and set the mode mode after successful decoding.

timofurrer · 2018-06-03T14:09:25Z

Why test_decodetwice was changed? Seems this breaks the purpose of this test.

I see, there is something off with the current test implementation - I'll have a look at it.
However, all those tests have side-effects - that's why I needed to make this change in the first place.
The side-effect is actually confirmed by the current implementation, because the uu.Error is still raised ..

Try to create a file with a mode 0o444 and check that the resulting file is read-only

Why is that? I could argue that this would be a test for os.chmod itself and I don't want the uu test to fail just because something is off with os.chmod ...
Any advice on how you want to handle such cases usually?

timofurrer · 2018-06-05T20:14:53Z

@serhiy-storchaka I've seen that you've been working on the tests ..
I've rebased and implemented a test without using a mock.

vstinner · 2018-06-06T10:37:53Z

It seems like your PR breaks test_uu on Windows (AppVeyor).

timofurrer · 2018-06-06T10:56:15Z

Unfortunately, I don't have access to a Windows. Do you know if os.access / os.W_OK behaves differently on a Windows? I couldn't find anything in the docs.
It's seems like os.chmod with 0o444 doesn't make the file read-only?

berkerpeksag · 2018-06-06T11:19:46Z

The test has failed on Linux too: https://python.visualstudio.com/cpython/_build?buildId=6360&tab=details&_a=summary

This line returns True:

self.assertFalse(os.access(self.tmpout, os.W_OK))

berkerpeksag · 2018-06-06T11:33:19Z

By the way, the os.chmod() call in decode() only triggered when out_file passed as str. I think you need to pass self.tmpout to decode() in line 228:

with open(self.tmpin, 'rb') as f:
    uu.decode(f)

timofurrer · 2018-06-09T08:53:31Z

By the way, the os.chmod() call in decode() only triggered when out_file passed as str. I think you need to pass self.tmpout to decode() in line 228:

This is not always the case, because at

cpython/Lib/uu.py

Line 124 in 1bcb8a6

out_file = hdrfields[2].rstrip(b' \t\r\n\f').decode("ascii")

out_file is set to the out_file encoded into the in_file. And this actually happens before the check if out_file is str or not.

With which user are the tests running?
I can reproduce the failure on my machine by running the tests as root:

sudo ./python Lib/test/test_uu.py -v

... which would explain the seen behavior on CI.

timofurrer · 2018-06-09T09:25:30Z

I've now replace the os.access() calls with os.stat() to verify the os.chmod call - Which is guess is fine for the test because for the test itself it doesn't matter what permissions are set - they just have to match.

serhiy-storchaka · 2018-06-10T07:35:14Z

Just try to open a file for writing. It should fail.

timofurrer · 2018-06-10T10:34:32Z

Just try to open a file for writing. It should fail.

That's actually not the case as you can see in: https://python.visualstudio.com/cpython/_build?buildId=8085

The only way I could explain this behavior is that the tests are run a root.

berkerpeksag · 2018-06-10T11:14:41Z

According to https://python.visualstudio.com/cpython/_build?buildId=8085 (click to "Display build info") UID is indeed 0.

berkerpeksag · 2018-06-10T11:16:39Z

Lib/test/test_uu.py


-if __name__=="__main__":
+if __name__ == "__main__":


I know this may be done by your editor/IDE automatically, but please don't include cosmetic changes to the PR.

I've reverted it. When I see something like that, should I create a separate PR?

We tend to reject cosmetic-only PRs, but if you find similar cases in Lib/test/ (not in the standard library) and have time to prepare a PR, it may be accepted.

timofurrer · 2018-06-10T11:22:05Z

According to https://python.visualstudio.com/cpython/_build?buildId=8085 (click to "Display build info") UID is indeed 0.

Would be nice to have a separate user to run the tests ..

berkerpeksag · 2018-06-10T11:58:53Z

uu is an ancient module and no one has noticed this for the past ~20 years, so perhaps we can just skip the test if UID is 0?

Lib/test/test_uu.py

vstinner · 2018-10-19T23:33:29Z

@berkerpeksag, @serhiy-storchaka: Berker approved this change, but it's still not merged. Is it ready to be merged? Since last Serhiy comments, updated have been pushed.

timofurrer · 2019-01-17T12:41:25Z

Is there anything missing from my side to get this merged?

miss-islington · 2019-01-17T14:15:56Z

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

bedevere-bot · 2019-01-17T14:16:12Z

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

(cherry picked from commit 17f05bb) Co-authored-by: Timo Furrer <tuxtimo@gmail.com>

berkerpeksag · 2019-01-17T14:17:28Z

@timofurrer sorry for my late response and thank you for your contribution and patience! :)

(cherry picked from commit 17f05bb) Co-authored-by: Timo Furrer <tuxtimo@gmail.com>

the-knights-who-say-ni added the CLA signed label May 31, 2018

bedevere-bot added the awaiting review label May 31, 2018

timofurrer force-pushed the bugfix/issue-33687 branch from 8d1f4b3 to f5f8168 Compare May 31, 2018

berkerpeksag reviewed May 31, 2018

View changes

timofurrer force-pushed the bugfix/issue-33687 branch 2 times, most recently from d8cea61 to 3fc26d2 Compare Jun 5, 2018

vstinner changed the title ~~bpo-33687: fix call to os.chmod~~ bpo-33687: uu: fix call to os.chmod Jun 6, 2018

timofurrer force-pushed the bugfix/issue-33687 branch from 3fc26d2 to 48fb285 Compare Jun 9, 2018

timofurrer force-pushed the bugfix/issue-33687 branch from f607979 to 56211ef Compare Jun 10, 2018

timofurrer force-pushed the bugfix/issue-33687 branch from 56211ef to 49939d5 Compare Jun 10, 2018

berkerpeksag reviewed Jun 10, 2018

View changes

timofurrer force-pushed the bugfix/issue-33687 branch from 49939d5 to c65cf29 Compare Jun 10, 2018

timofurrer force-pushed the bugfix/issue-33687 branch 2 times, most recently from a67107d to f6f6e42 Compare Jun 10, 2018

timofurrer force-pushed the bugfix/issue-33687 branch 2 times, most recently from 8614a61 to e2d8bac Compare Jun 10, 2018

berkerpeksag approved these changes Jun 10, 2018

View changes

bedevere-bot added awaiting merge and removed awaiting review labels Jun 10, 2018

timofurrer force-pushed the bugfix/issue-33687 branch from e2d8bac to 1b73f9a Compare Jun 10, 2018

serhiy-storchaka reviewed Jun 10, 2018

View changes

Lib/test/test_uu.py Outdated Show resolved Hide resolved

bpo-33687: fix call to os.chmod

7b12978

timofurrer force-pushed the bugfix/issue-33687 branch from 1b73f9a to 7b12978 Compare Jun 10, 2018

News entry must be one paragraph

beea790

serhiy-storchaka approved these changes Oct 20, 2018

View changes

berkerpeksag added the needs backport to 3.7 label Jan 17, 2019

berkerpeksag merged commit 17f05bb into python:master Jan 17, 2019

bedevere-bot removed awaiting merge needs backport to 3.7 labels Jan 17, 2019

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 17, 2019

bpo-33687: Fix call to os.chmod() in uu.decode() (pythonGH-7282)

84aa357

(cherry picked from commit 17f05bb) Co-authored-by: Timo Furrer <tuxtimo@gmail.com>

berkerpeksag pushed a commit that referenced this pull request Jan 17, 2019

bpo-33687: Fix call to os.chmod() in uu.decode() (GH-7282)

a261b73

(cherry picked from commit 17f05bb) Co-authored-by: Timo Furrer <tuxtimo@gmail.com>

bpo-33687: uu: fix call to os.chmod #7282

bpo-33687: uu: fix call to os.chmod #7282

timofurrer commented May 31, 2018 •

edited by bedevere-bot

berkerpeksag May 31, 2018

timofurrer May 31, 2018

serhiy-storchaka commented Jun 3, 2018

timofurrer commented Jun 3, 2018

timofurrer commented Jun 5, 2018

vstinner commented Jun 6, 2018

timofurrer commented Jun 6, 2018

berkerpeksag commented Jun 6, 2018 •

edited

berkerpeksag commented Jun 6, 2018

timofurrer commented Jun 9, 2018 •

edited

timofurrer commented Jun 9, 2018

serhiy-storchaka commented Jun 10, 2018

timofurrer commented Jun 10, 2018

berkerpeksag commented Jun 10, 2018

berkerpeksag Jun 10, 2018

timofurrer Jun 10, 2018

berkerpeksag Jun 10, 2018

timofurrer commented Jun 10, 2018

berkerpeksag commented Jun 10, 2018

vstinner commented Oct 19, 2018

timofurrer commented Jan 17, 2019

miss-islington commented Jan 17, 2019

bedevere-bot commented Jan 17, 2019

berkerpeksag commented Jan 17, 2019

bpo-33687: uu: fix call to os.chmod #7282

bpo-33687: uu: fix call to os.chmod #7282

Conversation

timofurrer commented May 31, 2018 • edited by bedevere-bot

berkerpeksag May 31, 2018

Choose a reason for hiding this comment

timofurrer May 31, 2018

Choose a reason for hiding this comment

serhiy-storchaka commented Jun 3, 2018

timofurrer commented Jun 3, 2018

timofurrer commented Jun 5, 2018

vstinner commented Jun 6, 2018

timofurrer commented Jun 6, 2018

berkerpeksag commented Jun 6, 2018 • edited

berkerpeksag commented Jun 6, 2018

timofurrer commented Jun 9, 2018 • edited

timofurrer commented Jun 9, 2018

serhiy-storchaka commented Jun 10, 2018

timofurrer commented Jun 10, 2018

berkerpeksag commented Jun 10, 2018

berkerpeksag Jun 10, 2018

Choose a reason for hiding this comment

timofurrer Jun 10, 2018

Choose a reason for hiding this comment

berkerpeksag Jun 10, 2018

Choose a reason for hiding this comment

timofurrer commented Jun 10, 2018

berkerpeksag commented Jun 10, 2018

vstinner commented Oct 19, 2018

timofurrer commented Jan 17, 2019

miss-islington commented Jan 17, 2019

bedevere-bot commented Jan 17, 2019

berkerpeksag commented Jan 17, 2019

timofurrer commented May 31, 2018 •

edited by bedevere-bot

berkerpeksag commented Jun 6, 2018 •

edited

timofurrer commented Jun 9, 2018 •

edited