gh-135532: optimize calls to PyMem_Malloc in SHAKE digest computation#135744
Conversation
|
!buildbot FIPS only |
|
🤖 New build scheduled with the buildbot fleet by @picnixz for commit de09602 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135744%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
de09602 to
7852a46
Compare
|
!buildbot FIPS only |
|
🤖 New build scheduled with the buildbot fleet by @picnixz for commit 7852a46 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135744%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
|
I should have thought a bit more but I don't know why we have a hard limit at 2**29. It looks like we're reserving 4 bits for the output length, just in case, but we should be able to use a digest of arbitrary length. There was a possible of overflows in 3.6 but this was fixed, and since then, as we're using OpenSSL, we don't have that issue anymore. And HACL* only requires the length to be an uint32_t. So, I think we should limit ourselves to uint32_t in both cases, even when using OpenSSL. It doesn't make sense to have a digest of size 2**32. SHAKE-128 is 128-bit secure with 32 bytes while SHAKE-256 is 256-bit secure with 64 bytes for the output length. Now, for OpenSSL's default provider, we have: int EVP_DigestFinalXOF(EVP_MD_CTX *ctx, unsigned char *md, size_t size)
{
int ret = 0;
OSSL_PARAM params[2];
size_t i = 0;
if (ctx->digest == NULL) {
ERR_raise(ERR_LIB_EVP, EVP_R_INVALID_NULL_ALGORITHM);
return 0;
}
if (ctx->digest->prov == NULL)
goto legacy;
if (ctx->digest->dfinal == NULL) {
ERR_raise(ERR_LIB_EVP, EVP_R_FINAL_ERROR);
return 0;
}
if ((ctx->flags & EVP_MD_CTX_FLAG_FINALISED) != 0) {
ERR_raise(ERR_LIB_EVP, EVP_R_FINAL_ERROR);
return 0;
}
/*
* For backward compatibility we pass the XOFLEN via a param here so that
* older providers can use the supplied value. Ideally we should have just
* used the size passed into ctx->digest->dfinal().
*/
params[i++] = OSSL_PARAM_construct_size_t(OSSL_DIGEST_PARAM_XOFLEN, &size);
params[i++] = OSSL_PARAM_construct_end();
if (EVP_MD_CTX_set_params(ctx, params) >= 0)
ret = ctx->digest->dfinal(ctx->algctx, md, &size, size);
ctx->flags |= EVP_MD_CTX_FLAG_FINALISED;
return ret;
legacy:
if (EVP_MD_xof(ctx->digest)
&& size <= INT_MAX
&& ctx->digest->md_ctrl(ctx, EVP_MD_CTRL_XOF_LEN, (int)size, NULL)) {
ret = ctx->digest->final(ctx, md);
if (ctx->digest->cleanup != NULL) {
ctx->digest->cleanup(ctx);
EVP_MD_CTX_set_flags(ctx, EVP_MD_CTX_FLAG_CLEANED);
}
OPENSSL_cleanse(ctx->md_data, ctx->digest->ctx_size);
} else {
ERR_raise(ERR_LIB_EVP, EVP_R_NOT_XOF_OR_INVALID_LENGTH);
}
return ret;
}So with a NULL provider, we expect
Thus, we can just require the digest length to fit on an |
b69e8e4 to
f885e8c
Compare
4c5de6e to
b26da82
Compare
3b49ff6 to
5a83082
Compare
This comment was marked as resolved.
This comment was marked as resolved.
5a83082 to
fc54897
Compare
|
!buildbot FIPS only |
|
🤖 New build scheduled with the buildbot fleet by @picnixz for commit fc54897 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F135744%2Fmerge The command will test the builders whose names match following regular expression: The builders matched are:
|
…putation (python#135744) - Add a fast path when the digest length is 0 to avoid calling useless functions. - Directly allocate via `PyBytes_FromStringAndSize(NULL, length)` when possible.
…putation (python#135744) - Add a fast path when the digest length is 0 to avoid calling useless functions. - Directly allocate via `PyBytes_FromStringAndSize(NULL, length)` when possible.
…putation (python#135744) - Add a fast path when the digest length is 0 to avoid calling useless functions. - Directly allocate via `PyBytes_FromStringAndSize(NULL, length)` when possible.
| } | ||
|
|
||
| CHECK_HACL_UINT32_T_LENGTH(length); | ||
| PyObject *digest = PyBytes_FromStringAndSize(NULL, length); |
There was a problem hiding this comment.
You should check for error (NULL) to detect memory allocation failure. I wrote #138923 which fix indirectly the issue.
Uh oh!
There was an error while loading. Please reload this page.