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

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

Merged
merged 2 commits into from
Jan 17, 2019

Conversation

timofurrer
Copy link
Contributor

@timofurrer timofurrer commented May 31, 2018

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

Lib/uu.py Outdated
@@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@serhiy-storchaka
Copy link
Member

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
Copy link
Contributor Author

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 timofurrer force-pushed the bugfix/issue-33687 branch 2 times, most recently from d8cea61 to 3fc26d2 Compare June 5, 2018 20:13
@timofurrer
Copy link
Contributor Author

@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
Copy link
Member

vstinner commented Jun 6, 2018

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

@vstinner vstinner changed the title bpo-33687: fix call to os.chmod bpo-33687: uu: fix call to os.chmod Jun 6, 2018
@timofurrer
Copy link
Contributor Author

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
Copy link
Member

berkerpeksag commented Jun 6, 2018

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
Copy link
Member

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
Copy link
Contributor Author

timofurrer commented Jun 9, 2018

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

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 timofurrer force-pushed the bugfix/issue-33687 branch from 3fc26d2 to 48fb285 Compare June 9, 2018 09:22
@timofurrer
Copy link
Contributor Author

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
Copy link
Member

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

@timofurrer
Copy link
Contributor Author

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.

@timofurrer timofurrer force-pushed the bugfix/issue-33687 branch from f607979 to 56211ef Compare June 10, 2018 10:55
@berkerpeksag
Copy link
Member

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

@timofurrer timofurrer force-pushed the bugfix/issue-33687 branch from 56211ef to 49939d5 Compare June 10, 2018 11:15

if __name__=="__main__":
if __name__ == "__main__":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 timofurrer force-pushed the bugfix/issue-33687 branch from 49939d5 to c65cf29 Compare June 10, 2018 11:20
@timofurrer
Copy link
Contributor Author

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 ..

@timofurrer timofurrer force-pushed the bugfix/issue-33687 branch 2 times, most recently from a67107d to f6f6e42 Compare June 10, 2018 11:38
@berkerpeksag
Copy link
Member

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?

@timofurrer timofurrer force-pushed the bugfix/issue-33687 branch 2 times, most recently from 8614a61 to e2d8bac Compare June 10, 2018 12:15
@timofurrer timofurrer force-pushed the bugfix/issue-33687 branch from 1b73f9a to 7b12978 Compare June 10, 2018 18:50
@vstinner
Copy link
Member

@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
Copy link
Contributor Author

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

@berkerpeksag berkerpeksag merged commit 17f05bb into python:master Jan 17, 2019
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 17, 2019
(cherry picked from commit 17f05bb)

Co-authored-by: Timo Furrer <tuxtimo@gmail.com>
@berkerpeksag
Copy link
Member

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

berkerpeksag pushed a commit that referenced this pull request Jan 17, 2019
(cherry picked from commit 17f05bb)

Co-authored-by: Timo Furrer <tuxtimo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants