bpo-32903: Fix a memory leak in os.chdir() on Windows by izbyshev · Pull Request #5801 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation21 Commits9 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Should I create a bug report for such simple fixes?

Choose a reason for hiding this comment

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

Makes sense. But I suggest removing the empty lines to keep code style consistent. And it's better to return True instead of return result in UNC condition. Also, add a NEWs entry please.

Alexey Izbyshev added 3 commits

February 28, 2018 10:58

GetCurrentDirectory() can't return non-normalized paths.

@zhangyangyu Thank you for reviewing! I've removed the empty lines and also realized that checking for // prefix doesn't make sense.

And it's better to return True instead of return result in UNC condition

Do you mean that SetEnvironmentVariable return value should be ignored?

Do you mean that SetEnvironmentVariable return value should be ignored?

@izbyshev , No, I mean before when it's a UNC path, TRUE is returned. Now, result(the return value of GetCurrentDirectoryW) is returned. Although usually there should be no difference, it's better not to do this. I have no idea about the comparison.

Although usually there should be no difference, it's better not to do this.

Actually, there must never be any difference, otherwise it is a bug (if result is 0 after GetCurrentDirectoryW, we shouldn't even check for UNC condition). I can add assert(result) before checking for UNC, but IMO changing the code to explicitly return TRUE only in case of UNC path would unnecessarily complicate it.

but IMO changing the code to explicitly return TRUE only in case of UNC path would unnecessarily complicate it.

The function is declared to return BOOL, so it's expected to return TRUE or FALSE. If you want to return any integer, just declare it as int. In the extreme case, is it my fault or the API's fault if result == TRUE is false for a function declared BOOL?

@zhangyangyu I totally misunderstood your point, sorry. I thought you were arguing that result may somehow be zero before UNC condition check.

If you worry about somebody using BOOL constants for testing for truth, I think it's better to simply use int since the only caller uses int for its own result.

Don't change the signature plz. Just add another check to let it return TRUE. It's not that bad.

Alexey Izbyshev added 2 commits

March 1, 2018 07:21

… like that"

This reverts commit 239d77e.

@zhangyangyu // or even /\ can be used when you pass paths to Win32 APIs, but you can't get them back from GetCurrentDirectory because they are normalized.

If you want to go deeper, the original is_unc_path name is also misleading because a path starting with \\ is not always a UNC path. It could also be a "local device" path (\\.\nul) or a "root local device" path (\\?\C:\). See this post if you're interested. So I've changed the test to be more explicit.

Regarding return is_unc_path ? TRUE : result, this would violate your own concern about returning an int from a function that should return BOOL.

Okay. But I would suggest changing the checks by a new issue if you are sure and asking our Windows gurus to decide, I really can't decide on that since I don't Windows :-(. Leave this issue only about the memory issue.

Regarding return is_unc_path ? TRUE : result, this would violate your own concern about returning an int from a function that should return BOOL.

Why? if is_unc_path is true, we always return TRUE now, if not, result is always the return value of SetEnvironmentVariableW, a BOOL. But wait, the Windows doc of it says it's return value is a just a nonzero but not TRUE. :-(

@zhangyangyu

Leave this issue only about the memory issue.

I wanted to take the opportunity to perform a minor cleanup by changing path checks since it doesn't distract from this small issue. But if you insist, I can revert the changes and use the original checks.

Why? if is_unc_path is true, we always return TRUE now, if not, result is always the return value of SetEnvironmentVariableW, a BOOL. But wait, the Windows doc of it says it's return value is a just a nonzero but not TRUE. :-(

If you care about explicit code, the type of result is int, and I don't think that making a person track data flow to verify that it's in fact can only be assigned to a BOOL value at the particular program point is explicit, especially since it involves looking at MSDN or headers to check SetEnvironmentVariableW signature. :)

Sorry for much bikeshedding. But believe me it's not a good idea to do the cleanup in this issue. Feel free to open a new issue to push that. Of course, thanks for this contribution!

Thank you for reviewing. I don't think it's worth opening a new issue for cleanup since the checks are correct, they are just a bit less clean and optimal than they could be.

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Mar 1, 2018

(cherry picked from commit 3e197c7)

Co-authored-by: Alexey Izbyshev izbyshev@users.noreply.github.com

Sorry, @izbyshev and @zhangyangyu, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 3e197c7a6740d564ad52fb7901c07d5ff49460f5 2.7

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Mar 1, 2018

(cherry picked from commit 3e197c7)

Co-authored-by: Alexey Izbyshev izbyshev@users.noreply.github.com

izbyshev added a commit to izbyshev/cpython that referenced this pull request

Mar 1, 2018

…-5801).

(cherry picked from commit 3e197c7)

Co-authored-by: Alexey Izbyshev izbyshev@users.noreply.github.com

zhangyangyu pushed a commit that referenced this pull request

Mar 1, 2018