gh-130151: Fix reference leaks in _hashlib.hmac_{new,digest}

[picnixz]

• Issue: Reference leaks in _hashlib.hmac_new and _hashlib.hmac_digest #130151

[encukou]

Thanks, good catch!

Why reorganize the code, though?
I see no reason to remove the arg name comment /*impl*/, or rename r.

AFAICS, declaring pointers at the top and setting them to NULL, and an error: block at the end, was deliberate; the style this function was going for is keeping ctx and self non-NULL when they need to be freed. IMO, that's a good way to make complex functions maintainable: repeating a list of currently owned “resources” before each return works for smaller functions, but I think hmac_new is past that size.
I'd expect a Py_XDECREF(self) in the error block, and a line like ctx = NULL; // context is now owned by self.

Not sure about the change from _setException to PyErr_NoMemory -- the OpenSSL docs on HMAC_CTX_new and HMAC_Init_ex aren't explicit on whether it sets an ERR_ error. Are they inconsistent?

[picnixz]

Not sure about the change from _setException to PyErr_NoMemory -- the OpenSSL docs on HMAC_CTX_new and

I think it's to align with other instances of similar code. In other cases I think we raise a MemoryError. But I need to check it.

Why reorganize the code, though?
I see no reason to remove the arg name comment /impl/, or rename r.

I think the impl comment would be implicit but it's my IDE, I'll restore it. As for r I think I wanted to reduce the kjne length :') I'll just revert this line.

AFAICS, declaring pointers at the top and setting them to NULL, and an error: block at the end, was deliberate

For me it was more an ANSI C approach where all variables are declared at the top of the function. However since I felt it was cleaner like this. But I can revert it and make it ANSI C.

[picnixz]

Concerning PyErr_NoMemory, that's what we do in hmac.digest(). NULL is returned by HMAC_CTX_new when allocation fails: https://github.com/openssl/openssl/blob/0bdd10e4078beccaa49ea015b6660f3facfab02b/crypto/hmac/hmac.c#L162.

Note that we also return NULL if the context cannot be reset correctly, but this only means that one the underlying EVP contexts couldn't be allocated either: https://github.com/openssl/openssl/blob/0bdd10e4078beccaa49ea015b6660f3facfab02b/crypto/hmac/hmac.c#L198 and https://github.com/openssl/openssl/blob/0bdd10e4078beccaa49ea015b6660f3facfab02b/crypto/evp/digest.c#L129.

So the NULL should only be returned by HMAC_CTX_new if either it couldn't be allocated or one of the underlying EVP contexts couldn't be allocated.

EDIT: This is assuming we are using openssl implementation for libssl. Any other implementation may not necessarily do the same, but I'm not sure we're actually building against something that is not openssl-compliant.

[miss-islington-app]

Thanks @picnixz for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

[bedevere-app]

GH-130491 is a backport of this pull request to the 3.13 branch.

[miss-islington-app]

Sorry, @picnixz and @gpshead, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 071820113f11b8f6a21f98652d0840e10268114c 3.12

[picnixz]

I'll take care of the bp tomorrow (it's 1 AM here) if no one is doing it

[picnixz]

Huh, I forgot. I didn't have time this morning. I'll do it... well, either this night or tomorrow. Sorry :(

[bedevere-app]

GH-130539 is a backport of this pull request to the 3.12 branch.