bpo-32903: Fix a memory leak in os.chdir() on Windows

[izbyshev]

https://bugs.python.org/issue32903

[izbyshev]

Should I create a bug report for such simple fixes?

[zhangyangyu]

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.

[izbyshev]

@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?

[zhangyangyu]

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.

[izbyshev]

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.

[zhangyangyu]

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?

[izbyshev]

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

[zhangyangyu]

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

[izbyshev]

@zhangyangyu Done!

[zhangyangyu]

@izbyshev Seems you can't remove the check for //: https://github.com/python/cpython/blob/master/Lib/pathlib.py#L127. I could find some other samples by searching that UNC starts with forward slashes.

And the code would be more explicit if you still use is_unc_path and return is_unc_path ? TRUE : result.

[izbyshev]

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

[izbyshev]

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

[zhangyangyu]

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

[izbyshev]

@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. :)

[zhangyangyu]

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!

[izbyshev]

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.

[miss-islington]

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.
🐍🍒⛏🤖

[bedevere-bot]

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

[miss-islington]

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

[bedevere-bot]

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

[bedevere-bot]

GH-5947 is a backport of this pull request to the 2.7 branch.