bpo-30769: Fix reference leak introduced in 77703942c59

[ericvw]

New error condition paths were introduced, which did not decrement
key2 and val2 objects. Therefore, decrement references before
jumping to the error label.

Signed-off-by: Eric N. Vander Weele ericvw@gmail.com

[willingc]

Thanks @ericvw for the PR on this issue. @emilyemorehouse, if you have time to review this PR, that would be great since you identified the added test that surfaced the leak (no worries if you don't have time).

[emilyemorehouse]

@willingc got it!
LGTM, I pulled it down and verified. I also double checked that 2.7 is still error free as a similar patch was applied from the original PR.

I'm happy to update the stage in the bug tracker as PRs progress as well, let me know if that would be helpful. (I also was not sure the exact difference between patch review and commit review).

[willingc]

Approving changes based on review by @emilyemorehouse.

[willingc]

Thanks @emilyemorehouse! I've approved this PR based on your review.

[vstinner]

Hum, I fail to find your name (Eric N. Vander Weele) in Misc/ACKS.

Can you please add yourself to Misc/ACKS?

By the way, @berkerpeksag forgot to add you when he pushed the commit ef6bf4c ;-)

[vstinner]

"@emilyemorehouse: LGTM"

Don't hesitate to approve the change ;-)

[vstinner]

Need to complete Misc/ACKS.

[emilyemorehouse]

@Haypo I'll be more explicit in the future. (:

I'll also keep an eye on this to ensure backporting is completed.

[berkerpeksag]

By the way, @berkerpeksag forgot to add you when he pushed the commit ef6bf4c ;-)

FTR, I didn't because it was a trivial patch. There is a note in devguide that says we should update Misc/ACKS only for nontrivial changes.

[vstinner]

@berkerpeksag: "FTR, I didn't because it was a trivial patch. There is a note in devguide that says we should update Misc/ACKS only for nontrivial changes."

Ok, it makes sense!

[ericvw]

Can you please add yourself to Misc/ACKS?

@Haypo, updated Misc/ACKS.

[vstinner]

Thanks @ericvw, I merged your fix! Anyone wants to backport the fix to 3.6 and 3.5?

[emilyemorehouse]

@Haypo I'm on it.

[bedevere-bot]

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

[emilyemorehouse]

3.5 has a merge conflict, looking into it. 😬

[emilyemorehouse]

Actually, the identical test_execve_invalid_env passes just fine on 3.5.

[vstinner]

@emilyemorehouse: "3.5 has a merge conflict, looking into it. 😬"

Do you need help to handle the conflict? You might rewrite the fix instead of trying a simple cherry-pick, it may be simpler.

[emilyemorehouse]

Rewriting the fix was indeed the path I was going to take. I'm stuck on not having a test case to illustrate the issue for 3.5 as the original test that I was using actually passes on 3.5 as is, though Serhiy pointed out that the bug does indeed exist for 3.5

[vstinner]

I just checked the 3.5 branch (at commit de1850b) on Linux, and I'm still able to reproduce the bug:

haypo@selma$ ./python -m test -R 3:3 -m test_execve_invalid_env test_os
0:00:00 load avg: 0.72 [1/1] test_os
beginning 6 repetitions
123456
......
test_os leaked [2, 2, 2] references, sum=6
test_os leaked [2, 2, 2] memory blocks, sum=6
1 test failed:
    test_os
Tests result: FAILURE

[bedevere-bot]

GH-2447 is a backport of this pull request to the 3.5 branch.