free cert_array if it is *not* None #1354
Conversation
|
This seems correct in my mind, can we get a test so we never hit this? |
|
Yeah hopefully @Lukasa can provide a suggestion on the best way to trigger this code path. Ultimately you only need to provide a |
|
Frankly the best way to test this is probably just to patch the method in a unit test and run the code. |
|
Sounds good to me. @reaperhulk could you create a unit test that hits this and ensures that the release occurs correctly? Then we can merge this! :) |
without the patch this test will crash
reaperhulk
force-pushed
the
dont-free-none
branch
from
f3a2f35
to
2d8c4d0
Compare
Codecov Report
@@ Coverage Diff @@
## master #1354 +/- ##
======================================
Coverage 100% 100%
======================================
Files 22 22
Lines 2030 2037 +7
======================================
+ Hits 2030 2037 +7
Continue to review full report at Codecov.
|
|
Test added! And sanity checked to confirm that it will crash the interpreter if the patch is not in place 😄 |
test/contrib/test_securetransport.py
Outdated
|
|
||
| def test_no_crash_with_empty_trust_bundle(): | ||
| try: | ||
| s = socket.socket() |
There was a problem hiding this comment.
with contextlib.closing(socket.socket()) as s: instead of try/finally
There was a problem hiding this comment.
I had no idea that existed. Very cool 😄 Updated.
|
Anything needed from me on this PR still? Test failures appear unrelated 😢 |
Refs pypa/pip#5131
This fixes both a crash if
cert_arrayisNoneand a memory leak if it is not.