bpo-37702: Fix SSL certificate-store-handles leak #14999
Conversation
In Windows, CertCloseStore(hCollectionStore) closes only hCollectionStore. To avoid out-of-memory, CertCloseStore() shuld be called to each hSystemStore which was added to hCollectionStore.
|
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. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
|
The theory seems fine. I haven't checked the implementation - can't say when I'll get to it, but I'll try and do it before the next release. |
Misc/NEWS.d/next/Windows/2019-07-29-16-49-31.bpo-37702.Lj2f5e.rst
Outdated
Show resolved
Hide resolved
Co-Authored-By: Ashwin Ramaswami <[email protected]>
Modules/_ssl.c
Outdated
| associated with the store, in this case our collection store and the | ||
| associated system stores. */ | ||
| if (!CertCloseStore(hCollectionStore, CERT_CLOSE_STORE_FORCE_FLAG)) { | ||
| /* When a collection store and its sibling stores are closed |
There was a problem hiding this comment.
Do you think it would be good to keep in the definition of CERT_CLOSE_STORE_FORCE_FLAG ("forces freeing of memory for all contexts associated with the store")?
There was a problem hiding this comment.
Thanks for Bool position.
For now, both 0 and CERT_CLOSE_STORE_FORCE_FLAG work fine for my pc.
I'd like to keep changes to a minimum and leave it to other ssl experts' judgement whether the flag should revert to 0 or not.
There was a problem hiding this comment.
Oh, I didn't mean reverting the flag to 0, I meant, did you want to keep the part in the comment that says "CERT_CLOSE_STORE_FORCE_FLAG forces freeing of memory for all contexts associated with the store"?
There was a problem hiding this comment.
MS says,
CERT_CLOSE_STORE_FORCE_FLAG:
Forces the freeing of memory for all contexts associated with the store.
This flag can be safely used only when the store is opened in a function
and neither the store handle nor any of its contexts are passed to
any called functions. For details, see ...
I think, keeping short sentences caused and will cause a misunderstanding that
this rarely used flag is always safe and releases all certificate-store-handles.
Having said that, I'm not in favor of long description in the source file.
MS doesn't recommend using CERT_CLOSE_STORE_FORCE_FLAG. https://blogs.msdn.microsoft.com/winsdk/2010/01/29/passing-the-flag-cert_close_store_force_flag-to-certclosestore-may-cause-your-application-to-crash/ And I don't want to leave confusing comments on the flag that is no longer necessary to close all handles. So, I decided to change the flag back to 0.
|
MS doesn't recommend using CERT_CLOSE_STORE_FORCE_FLAG. And I don't want to leave confusing comments on the flag that is no longer necessary to close all handles. |
|
I'm done. Sorry for the dirty PR. |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
Please check. |
|
Thanks for making the requested changes! @tiran: please review the changes made to this pull request. |
Not necessary for now.
|
By any chance, Does this PR for Windows need further review from zooba? |
|
I raised an alternative PR, which fix store leaks rather than handle leaks. |
|
I'm closing this as it's unnecessary with the other fix (I verified there is no handle leak or memory leak with |
In Windows, CertCloseStore(hCollectionStore,[any option]) closes only hCollectionStore.
To avoid out-of-memory, CertCloseStore() shuld be called to each hSystemStore
which was added to hCollectionStore.
I think CERT_CLOSE_STORE_FORCE_FLAG is rare option and not much needed.
MSDN says CertCloseStore(handle, 0) is ideal and works fine for me.
https://bugs.python.org/issue37702