Thanks, good catch!

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

AFAICS, declaring pointers at the top and setting them toNULL, and anerror: block at the end, was deliberate; the style this function was going for is keepingctx andself 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 eachreturn works for smaller functions, but I thinkhmac_new is past that size.
I'd expect aPy_XDECREF(self) in the error block, and a line likectx = NULL; // context is now owned by self.

Not sure about the change from_setException toPyErr_NoMemory -- the OpenSSL docs onHMAC_CTX_new andHMAC_Init_ex aren't explicit on whether it sets anERR_ error. Are they inconsistent?

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

ConcerningPyErr_NoMemory, that's what we do inhmac.digest().NULL is returned byHMAC_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 andhttps://github.com/openssl/openssl/blob/0bdd10e4078beccaa49ea015b6660f3facfab02b/crypto/evp/digest.c#L129.

So the NULL should only be returned byHMAC_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 usingopenssl implementation forlibssl. Any other implementation may not necessarily do the same, but I'm not sure we're actually building against something that is notopenssl-compliant.

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

GH-130491 is a backport of this pull request to the3.13 branch.

Sorry,@picnixz and@gpshead, I could not cleanly backport this to3.12 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 071820113f11b8f6a21f98652d0840e10268114c 3.12

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

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

GH-130539 is a backport of this pull request to the3.12 branch.