Add array memory allocation routines #28059

esyr · 2025-07-17T13:30:22Z

This patch set adds variants of memory allocation routines suitable for array allocation, that perform an additional check for integer overflow during the size calculation. Also, in order to add a test for the functions, some cleanup of the existing memory allocation routines' behaviour has been done.

Checklist

documentation is added or updated
tests are added or updated

crypto/mem_sec.c

include/internal/check_size_overflow.h

ssl/ssl_cert.c

crypto/mem.c

levitte · 2025-07-17T14:08:16Z

The reason you have trouble in the FIPS provider is that you need to reproduce the new allocation calls in providers/fips/fipsprov.c, and also ensure the corresponding upcalls exist

paulidale · 2025-07-17T22:19:10Z

include/internal/check_size_overflow.h

+    if (ossl_unlikely(((num | size) >= HALF_SIZE_T)
+                      && size && ((*bytes / size) != num))) {


Did you look at using our existing overflow detection code in internal/safe_math.h for these calculations?

That code will be more efficient when compiler support is available (& it mostly is).

Our coding style suggests size != 0 instead of size.

Your test of (num | size) >= HALF_SIZE_T might be a worthwhile improvement to the safe_math.h unsigned multiplication code since it will avoid a division most of the time.

Did you look at using our existing overflow detection code in internal/safe_math.h for these calculations?

That code will be more efficient when compiler support is available (& it mostly is).

I haven't, will take a look, thanks!

So, I've looked into it, and it seems that it is unsuitable for use in a header, as the function names are not predetermined, and an invocation of OSSL_SAFE_MATH_SIGNED creates a bunch of functions that may later collide with the ones defined by its usage in the compilation units.

Why does the check have to be in a header?
Doing so feels like premature optimisation...

Originally it has been envisioned as a small snippet that can be then used in different files, as it really doesn't do much, just a variation of safe_mul, but with setting an error, so the allocation-related issues are easier to trace; otherwise, I don't see a suitable file for storing the implementation, do you have a suggestion?

paulidale · 2025-07-17T22:21:56Z

The reason you have trouble in the FIPS provider is that you need to reproduce the new allocation calls in providers/fips/fipsprov.c, and also ensure the corresponding upcalls exist

Alternatively, provide these new calls in a separate file that calls the normal memory allocation routines. Then build that file into the FIPS provider.

Another advantage of this is your header could be pulled into the file and the is_size_overflow() function made local to it & check_size_overflow.h removed.

include/internal/check_size_overflow.h

Sashan

I like this change. I did use find ./ -name "*.c" -o -name "*.h" -o -name "*.h.in" |xargs grep OPENSSL_.*alloc |egrep -e '\* +' to find more places which could be improved in existing code base. those are further suggestions which have popped up:

diff --git a/apps/x509.c b/apps/x509.c
index 6051d8642f..c9d26f8b20 100644
--- a/apps/x509.c
+++ b/apps/x509.c
@@ -1303,7 +1303,7 @@ static int print_x509v3_exts(BIO *bio, X509 *x, const char *ext_names)
         BIO_printf(bio, "Invalid extension names: %s\n", ext_names);
         goto end;
     }
-    if ((names = OPENSSL_malloc(sizeof(char *) * nn)) == NULL)
+    if ((names = OPENSSL_malloc_array(nn, sizeof(char *))) == NULL)
         goto end;
     parse_ext_names(tmp_ext_names, names);
 
diff --git a/fuzz/hashtable.c b/fuzz/hashtable.c
index 38d2295076..6fc033b3e6 100644
--- a/fuzz/hashtable.c
+++ b/fuzz/hashtable.c
@@ -103,7 +103,7 @@ int FuzzerInitialize(int *argc, char ***argv)
 
     OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CRYPTO_STRINGS, NULL);
     ERR_clear_error();
-    prediction_table = OPENSSL_zalloc(sizeof(FUZZER_VALUE) * 65537);
+    prediction_table = OPENSSL_calloc(65537, sizeof(FUZZER_VALUE));
     if (prediction_table == NULL)
         return -1;
     fuzzer_table = ossl_ht_new(&fuzz_conf);
diff --git a/providers/implementations/encode_decode/ml_common_codecs.c b/providers/implementations/encode_decode/ml_common_codecs.c
index 773550c9fb..97d37ecd10 100644
--- a/providers/implementations/encode_decode/ml_common_codecs.c
+++ b/providers/implementations/encode_decode/ml_common_codecs.c
@@ -42,7 +42,7 @@ ossl_ml_common_pkcs8_fmt_order(const char *algorithm_name,
     const char *sep = "\t ,";
 
     /* Reserve an extra terminal slot with fmt == NULL */
-    if ((ret = OPENSSL_zalloc((NUM_PKCS8_FORMATS + 1) * sizeof(*ret))) == NULL)
+    if ((ret = OPENSSL_calloc((NUM_PKCS8_FORMATS + 1), sizeof(*ret))) == NULL)
         return NULL;
 
     /* Entries that match a format will get a non-zero preference. */
diff --git a/providers/implementations/kdfs/argon2.c b/providers/implementations/kdfs/argon2.c
index e3f9aa4f0f..00305faaac 100644
--- a/providers/implementations/kdfs/argon2.c
+++ b/providers/implementations/kdfs/argon2.c
@@ -564,8 +564,8 @@ static int fill_mem_blocks_mt(KDF_ARGON2 *ctx)
     void **t;
     ARGON2_THREAD_DATA *t_data;
 
-    t = OPENSSL_zalloc(sizeof(void *)*ctx->lanes);
-    t_data = OPENSSL_zalloc(ctx->lanes * sizeof(ARGON2_THREAD_DATA));
+    t = OPENSSL_calloc(ctx->lanes, sizeof(void *));
+    t_data = OPENSSL_calloc(ctx->lanes, sizeof(ARGON2_THREAD_DATA));
 
     if (t == NULL || t_data == NULL)
         goto fail;

and of course all suggestions here can be addressed as follow up PRs. It's up to you to address it now, or later in follow up PRs. just let me know how you'll proceed and I will approve.

crypto/ec/ecp_nistz256.c

crypto/mem_sec.c

nhorman

looks like you need to run make update and commit the changes to util/libcrypto.num so that the new alloc call gets exported properly.

nhorman · 2025-07-23T16:26:23Z

Just FYI, since we discussed this on standup today, and there has been some concern over fips/provider compatibility here, I'm going to close and reopen this with the extended_tests label set, so that the provider compat tests get run.

Seems like the case of realloc() returning NULL with non-zero size has been overlooked. Complements: 5639ee7 "ERR: Make CRYPTO_malloc() and friends report ERR_R_MALLOC_FAILURE" Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Analogous to the way CRYPTO_malloc does it. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Complements: b51bce9 "Add and use OPENSSL_zalloc" Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Otherwise util/check-format-commit.sh complains about the wrong alignment. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Such routines allow alleviating the need to perform explicit integer overflow check during allocation size calculation and generally make the allocations more semantic (as they signify that a collection of NUM items, each occupying SIZE bytes is being allocated), which paves the road for additional correctness checks in the future. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Co-Authored-by: Alexandr Nedvedicky <sashan@openssl.org> Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Co-Authored-by: Alexandr Nedvedicky <sashan@openssl.org> Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

There are some *.inc already in the repository, mostly in demos/tests and related to some algorithm implementations. Introduction of array_alloc.inc has made including these files in the tags generation even more pertinent, so they are included now. Also, this commit explicitly marks *.h files as containing C code, overriding universal-ctags default of interpreting them as C++/ObjectiveC ones. Suggested-by: Neil Horman <nhorman@openssl.org> Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

* crypto/mem.c [OPENSSL_SMALL_FOOTPRINT] (CRYPTO_aligned_alloc): Change freeptr to *freeptr to properly update the variable passed by pointer. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

…gned_alloc There is no need to initialise neither *freeptr, as it is initialised already, nor ret, as NULL can be simply returned instead. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

The original C11 specification is pretty weird: it specifies that the size must be a multiple of alignment (rendering it useless for small page-aligned allocations that, for example, might be useful for RDMA) and until DR460[1] it was UB in failing to do so (as it is with OPENSSL_ligned_alloc() calls in alloc_new_neighborhood_list() on 32-bit systems, for example). Moreover, it has arguably not been used much before, as all supported POSIX systems have at least POSIX 2001 compatibility level nowadays, Windows doesn't implement aligned_alloc() at all (because implementation of free() in MS CRT is unable to handle aligned allocations[2]), and _ISOC11_SOURCE is a glibc-specific feature test macro. [1] https://open-std.org/JTC1/SC22/WG14/www/docs/summary.htm#dr_460 [2] https://learn.microsoft.com/en-us/cpp/standard-library/cstdlib?view=msvc-170#remarks-6 Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

…igned_alloc Otherwise the roundup calculation performed in the open-coded implementation may put the pointer out of bounds. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Report the errors for the known error codes returned by posix_memalign(). Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

…is used Per [1]: The value of alignment shall be a power of two multiple of sizeof(void *). [1] https://pubs.opengroup.org/onlinepubs/9799919799/functions/posix_memalign.html Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

…d_alloc The open-coded implementation performs addition of size and alignment, that may overflow. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

OPENSSL_aligned_alloc can return NULL in cases other than memory exhaustion or incorrect arguments, document that. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

…lures Explicitly document that it is set to NULL, so can be passed to free() without additional checks. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

…ng returned The semantics of OPENSSL_secure_[mz]alloc is somewhat unorthodox, as it silently return a pointer to non-secure memory if the arena is not initialised, which, while mentioned in the DESCRIPTION, is not clear from reading the pertaining part of the RETURNING VALUE section alone; explicitly state that the memory may be allocated by OPENSSL_calloc instead if the secure heap is not initialised. Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

So it is possible to change the allocator implementation, as it must be before the first malloc call. Suggested-by: Matt Caswell <matt@openssl.org> Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org> Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> Reviewed-by: Paul Dale <ppzgs1@gmail.com> Reviewed-by: Neil Horman <nhorman@openssl.org> (Merged from #28059)

…rray [1] has introduced array allocation routines, and, since those are available, it is prudent to use where they are appropriate, like in this example. [1] openssl/openssl#28059 Signed-off-by: Eugene Syromiatnikov <esyr@openssl.org>

esyr requested a review from levitte July 17, 2025 13:30

esyr force-pushed the esyr/calloc branch 2 times, most recently from 8476a94 to d564826 Compare July 17, 2025 13:40

levitte reviewed Jul 17, 2025

View reviewed changes

crypto/mem_sec.c Outdated Show resolved Hide resolved

levitte reviewed Jul 17, 2025

View reviewed changes

include/internal/check_size_overflow.h Outdated Show resolved Hide resolved

levitte reviewed Jul 17, 2025

View reviewed changes

ssl/ssl_cert.c Outdated Show resolved Hide resolved

levitte reviewed Jul 17, 2025

View reviewed changes

crypto/mem.c Outdated Show resolved Hide resolved

esyr force-pushed the esyr/calloc branch 2 times, most recently from 7c5db12 to cb96c58 Compare July 17, 2025 14:04

paulidale reviewed Jul 17, 2025

View reviewed changes

include/internal/check_size_overflow.h Outdated Show resolved Hide resolved

Sashan requested changes Jul 21, 2025

View reviewed changes

crypto/ec/ecp_nistz256.c Show resolved Hide resolved

crypto/mem_sec.c Outdated Show resolved Hide resolved

vavroch2010 added this to Development Board Jul 21, 2025

vavroch2010 moved this to Waiting Review in Development Board Jul 21, 2025

vavroch2010 assigned esyr Jul 21, 2025

vavroch2010 requested review from nhorman and Sashan July 21, 2025 15:27

nhorman requested changes Jul 21, 2025

View reviewed changes

esyr force-pushed the esyr/calloc branch from cb96c58 to 634d279 Compare July 23, 2025 11:02

github-actions bot added severity: fips change The pull request changes FIPS provider sources severity: ABI change This pull request contains ABI changes labels Jul 23, 2025

esyr force-pushed the esyr/calloc branch from 634d279 to a242289 Compare July 23, 2025 13:23

vavroch2010 requested a review from mattcaswell July 23, 2025 15:19

esyr force-pushed the esyr/calloc branch from 22ac175 to d236561 Compare July 23, 2025 15:32

nhorman added the extended tests Run extended tests in CI label Jul 23, 2025

nhorman closed this Jul 23, 2025

github-project-automation bot moved this from Waiting Review to Done in Development Board Jul 23, 2025

		if (ossl_unlikely(((num \| size) >= HALF_SIZE_T)
		&& size && ((*bytes / size) != num))) {

Uh oh!

Add array memory allocation routines #28059

Add array memory allocation routines #28059

Uh oh!

Conversation

esyr commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

levitte commented Jul 17, 2025

Uh oh!

paulidale Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

paulidale Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

paulidale Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esyr Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

esyr Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

paulidale Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

esyr Jul 24, 2025

Choose a reason for hiding this comment

Uh oh!

paulidale commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Sashan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nhorman left a comment

Choose a reason for hiding this comment

Uh oh!

nhorman commented Jul 23, 2025

Uh oh!

Uh oh!

esyr commented Jul 17, 2025 •

edited

Loading

paulidale Jul 17, 2025 •

edited

Loading

paulidale commented Jul 17, 2025 •

edited

Loading