Bruno Haible <[email protected]> writes: > Collin Funk wrote: >> 256 bytes seems like enough for the foreseeable future. > > I agree: that's more than 3x the 72 bytes that it currently needs. > >> I think it is okay to leave the abort () calls for EVP_DigestFinal_ex >> and EVP_DigestUpdate which seem like they should never fail outside of >> programmer error. For example, using a NULL pointer argument or calling >> them without calling EVP_DigestInit_ex. I rather abort () there than >> have a program print uninitialized memory or something like that. > > Agreed. These are the situations for which abort() is adequate. > >> What do you think of the attached patch? > > Looks essentially right, except that the casts > (EVP_MD_CTX *) &ctx->evp_ctx_buffer > open a possibility of alignment problems (ISO C 23 § 6.3.2.3.(7)). > You can't work around it by using a > union { uint8_t evp_ctx_buffer[256]; EVP_MD_CTX evp_ctx; } > since the latter is an incomplete type. Therefore IMO the best solution > is to use max_align_t from <stddef.h>: > > max_align_t evp_ctx_buffer[256 / sizeof (max_align_t)];
I had assumed that 256 bytes would be aligned everywhere, but max_align_t seams more robust, thanks. > Also, why write > (EVP_MD_CTX *) &ctx->evp_ctx_buffer > instead of > (EVP_MD_CTX *) ctx->evp_ctx_buffer > since it's an array anyway? Not sure why I wrote it that way, to be honest... > Finally, if sizeof (EVP_MD_CTX) some day gets increased > 256, we should be > able to detect it. I think it can be done through a separate unit test file, > that overrides the functions > malloc > calloc > realloc > (with dlsym RTLD_NEXT), in such a way that these overrides abort if the > size argument is > 256. This test should only be activated on __ELF__ > systems, because on the other ones (macOS, Windows, etc.) you probably > can't rely on dlsym RTLD_NEXT. Good idea. But wouldn't it be easier to steal coreutils gcc_shared_() function from init.cfg and use LD_PRELOAD? Here is an example [1]. Pushed the patch with your suggestions, and will work on that test later. Collin [1] https://github.com/coreutils/coreutils/commit/4bfcf62f74b38d762ee06ceef582c326023635a9
>From 06e7da510bd055f524c1baff0890c861bef2b6a4 Mon Sep 17 00:00:00 2001 Message-ID: <06e7da510bd055f524c1baff0890c861bef2b6a4.1756962839.git.collin.fu...@gmail.com> From: Collin Funk <[email protected]> Date: Tue, 2 Sep 2025 19:16:17 -0700 Subject: [PATCH v2] crypto/sha3-buffer: Don't abort on OOM. * modules/crypto/sha3-buffer (Depends-on): Add stddef-h. * lib/sha3.h: Include stddef.h. (sha3_ctx): Use a stack allocated buffer used to cast to store an EVP_MD_CTX. * lib/sha3.c (DEFINE_SHA3_INIT_CTX): Initialize the structure to zero. Use the stack allocated buffer instead of calling EVP_MD_CTX_create. (sha3_read_ctx): Just call sha3_finish_ctx similar to the other crypto modules. (sha3_finish_ctx): Remove call to EVP_MD_CTX_free. Call EVP_DigestFinal_ex. --- ChangeLog | 12 ++++++++++++ lib/sha3.c | 25 +++++++++++++------------ lib/sha3.h | 7 ++++--- modules/crypto/sha3-buffer | 1 + 4 files changed, 30 insertions(+), 15 deletions(-) diff --git a/ChangeLog b/ChangeLog index a016eec8f0..3752bb9198 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,17 @@ 2025-09-03 Collin Funk <[email protected]> + crypto/sha3-buffer: Don't abort on OOM. + * modules/crypto/sha3-buffer (Depends-on): Add stddef-h. + * lib/sha3.h: Include stddef.h. + (sha3_ctx): Use a stack allocated buffer used to cast to store an + EVP_MD_CTX. + * lib/sha3.c (DEFINE_SHA3_INIT_CTX): Initialize the structure to zero. + Use the stack allocated buffer instead of calling EVP_MD_CTX_create. + (sha3_read_ctx): Just call sha3_finish_ctx similar to the other crypto + modules. + (sha3_finish_ctx): Remove call to EVP_MD_CTX_free. Call + EVP_DigestFinal_ex. + gnulib-tool: Avoid Automake error when using --create-testdir (regr. today). * gnulib-tool.sh (func_emit_tests_Makefile_am): Initialize AM_CFLAGS and AM_CXXFLAGS when creating a test directory. diff --git a/lib/sha3.c b/lib/sha3.c index 5da85db5f2..e30210fb3e 100644 --- a/lib/sha3.c +++ b/lib/sha3.c @@ -323,11 +323,11 @@ sha3_process_block (const void *buffer, size_t len, struct sha3_ctx *ctx) void \ sha3_##SIZE##_init_ctx (struct sha3_ctx *ctx) \ { \ - int rc; \ - ctx->evp_ctx = EVP_MD_CTX_create (); \ - if (ctx->evp_ctx == NULL) \ - abort (); \ - rc = EVP_DigestInit_ex (ctx->evp_ctx, EVP_sha3_##SIZE (), NULL); \ + /* EVP_DigestInit_ex expects all bytes to be zero. */ \ + memset (ctx, 0, sizeof *ctx); \ + EVP_MD_CTX *evp_ctx = (EVP_MD_CTX *) ctx->evp_ctx_buffer; \ + int rc = EVP_DigestInit_ex (evp_ctx, EVP_sha3_##SIZE (), NULL); \ + /* This should never fail. */ \ if (rc == 0) \ abort (); \ } @@ -341,17 +341,17 @@ void * sha3_read_ctx (const struct sha3_ctx *ctx, void *resbuf) { /* Assume any unprocessed bytes in ctx are not to be ignored. */ - int result = EVP_DigestFinal_ex (ctx->evp_ctx, resbuf, NULL); - if (result == 0) - abort (); - return resbuf; + return sha3_finish_ctx ((struct sha3_ctx *) ctx, resbuf); } void * sha3_finish_ctx (struct sha3_ctx *ctx, void *resbuf) { - void *result = sha3_read_ctx (ctx, resbuf); - EVP_MD_CTX_free (ctx->evp_ctx); + EVP_MD_CTX *evp_ctx = (EVP_MD_CTX *) ctx->evp_ctx_buffer; + /* This should never fail. */ + int result = EVP_DigestFinal_ex (evp_ctx, resbuf, NULL); + if (result == 0) + abort (); return resbuf; } @@ -373,7 +373,8 @@ DEFINE_SHA3_BUFFER (512) void sha3_process_bytes (const void *buffer, size_t len, struct sha3_ctx *ctx) { - int result = EVP_DigestUpdate (ctx->evp_ctx, buffer, len); + EVP_MD_CTX *evp_ctx = (EVP_MD_CTX *) ctx->evp_ctx_buffer; + int result = EVP_DigestUpdate (evp_ctx, buffer, len); if (result == 0) abort (); } diff --git a/lib/sha3.h b/lib/sha3.h index 7de1216fda..adce43eac7 100644 --- a/lib/sha3.h +++ b/lib/sha3.h @@ -19,6 +19,7 @@ #ifndef SHA3_H # define SHA3_H 1 +# include <stddef.h> # include <stdio.h> # include <stdint.h> @@ -50,9 +51,9 @@ enum { SHA3_512_BLOCK_SIZE = 576 / 8 }; struct sha3_ctx { # if HAVE_OPENSSL_SHA3 - /* This is an incomplete type, so we can only place a pointer in the - struct. */ - EVP_MD_CTX *evp_ctx; + /* EVP_MD_CTX is an incomplete type. EVP_MD_CTX_create allocates 72 bytes of + memory as of 2025-09-02. */ + max_align_t evp_ctx_buffer[256 / sizeof (max_align_t)]; # else u64 state[25]; uint8_t buffer[144]; /* Up to BLOCKLEN in use. */ diff --git a/modules/crypto/sha3-buffer b/modules/crypto/sha3-buffer index 83b3a090ff..e0e654e563 100644 --- a/modules/crypto/sha3-buffer +++ b/modules/crypto/sha3-buffer @@ -10,6 +10,7 @@ m4/sha3.m4 Depends-on: byteswap c99 +stddef-h stdint-h u64 -- 2.51.0
