[musl] [PATCH v3] implement recallocarray(3)

[musl] [PATCH v3] implement recallocarray(3)

[Ariadne Conill]

This OpenBSD extension is similar to reallocarray(3), but
zero-initializes the new memory area.

This extension is placed in _BSD_SOURCE, like
reallocarray(3).

Changes from v2:
- drop overflow checking for old size

Changes from v1:
- use realloc() instead of reallocarray()
---
 include/stdlib.h           |  1 +
 src/malloc/recallocarray.c | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 src/malloc/recallocarray.c

diff --git a/include/stdlib.h b/include/stdlib.h
index b54a051f..a0412ad4 100644
--- a/include/stdlib.h
+++ b/include/stdlib.h
@@ -146,6 +146,7 @@ int clearenv(void);
 #define WCOREDUMP(s) ((s) & 0x80)
 #define WIFCONTINUED(s) ((s) == 0xffff)
 void *reallocarray (void *, size_t, size_t);
+void *recallocarray (void *, size_t, size_t, size_t);
 #endif
 
 #ifdef _GNU_SOURCE
diff --git a/src/malloc/recallocarray.c b/src/malloc/recallocarray.c
new file mode 100644
index 00000000..a7827604
--- /dev/null
+++ b/src/malloc/recallocarray.c
@@ -0,0 +1,27 @@
+#define _BSD_SOURCE
+#include <errno.h>
+#include <stdlib.h>
+#include <string.h>
+
+void *recallocarray(void *ptr, size_t om, size_t m, size_t n)
+{
+	void *newptr;
+	size_t old_size = om * n, new_size;
+
+	if (n && m > -1 / n) {
+		errno = ENOMEM;
+		return 0;
+	}
+	new_size = m * n;
+
+	if (new_size <= old_size) {
+		memset((char *) ptr + new_size, 0, old_size - new_size);
+	}
+
+	newptr = realloc(ptr, m * n);
+	if (new_size > old_size) {
+		memset((char *) ptr + old_size, 0, new_size - old_size);
+	}
+
+	return newptr;
+}
-- 
2.28.0

Re: [musl] [PATCH v3] implement recallocarray(3)

[Dmitry Samersoff]

Ariadne,

BSD (jemalloc) realloc always perform a new allocation and may return 
completely new pointer ever if new_size <= old_size.

Also contract for realloc says that if allocation fails original content 
should remain untouched.

I don't know how musl malloc perform in this case, but it might be 
better to move all memset after realloc and use newptr as a memeset 
base, with an appropriate error checking.

-Dmitry


On 02.08.2020 0:42, Ariadne Conill wrote:
> This OpenBSD extension is similar to reallocarray(3), but
> zero-initializes the new memory area.
> 
> This extension is placed in _BSD_SOURCE, like
> reallocarray(3).
> 
> Changes from v2:
> - drop overflow checking for old size
> 
> Changes from v1:
> - use realloc() instead of reallocarray()
> ---
>   include/stdlib.h           |  1 +
>   src/malloc/recallocarray.c | 27 +++++++++++++++++++++++++++
>   2 files changed, 28 insertions(+)
>   create mode 100644 src/malloc/recallocarray.c
> 
> diff --git a/include/stdlib.h b/include/stdlib.h
> index b54a051f..a0412ad4 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -146,6 +146,7 @@ int clearenv(void);
>   #define WCOREDUMP(s) ((s) & 0x80)
>   #define WIFCONTINUED(s) ((s) == 0xffff)
>   void *reallocarray (void *, size_t, size_t);
> +void *recallocarray (void *, size_t, size_t, size_t);
>   #endif
>   
>   #ifdef _GNU_SOURCE
> diff --git a/src/malloc/recallocarray.c b/src/malloc/recallocarray.c
> new file mode 100644
> index 00000000..a7827604
> --- /dev/null
> +++ b/src/malloc/recallocarray.c
> @@ -0,0 +1,27 @@
> +#define _BSD_SOURCE
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +void *recallocarray(void *ptr, size_t om, size_t m, size_t n)
> +{
> +	void *newptr;
> +	size_t old_size = om * n, new_size;
> +
> +	if (n && m > -1 / n) {
> +		errno = ENOMEM;
> +		return 0;
> +	}
> +	new_size = m * n;
> +
> +	if (new_size <= old_size) {
> +		memset((char *) ptr + new_size, 0, old_size - new_size);
> +	}
> +
> +	newptr = realloc(ptr, m * n);
> +	if (new_size > old_size) {
> +		memset((char *) ptr + old_size, 0, new_size - old_size);
> +	}
> +
> +	return newptr;
> +}
> 

Re: [musl] [PATCH v3] implement recallocarray(3)

[Markus Wichmann]

On Sat, Aug 01, 2020 at 03:42:16PM -0600, Ariadne Conill wrote:
> +void *recallocarray(void *ptr, size_t om, size_t m, size_t n)
> +{
> +	void *newptr;
> +	size_t old_size = om * n, new_size;
> +
> +	if (n && m > -1 / n) {
> +		errno = ENOMEM;
> +		return 0;
> +	}
> +	new_size = m * n;
> +
> +	if (new_size <= old_size) {
> +		memset((char *) ptr + new_size, 0, old_size - new_size);
> +	}
> +
> +	newptr = realloc(ptr, m * n);
> +	if (new_size > old_size) {
> +		memset((char *) ptr + old_size, 0, new_size - old_size);
> +	}
> +
> +	return newptr;
> +}

Use after free detected. Once realloc() returns, if newptr is not 0 and
ptr is not 0, then ptr is free (unless ptr happens to be equal to
newptr). Therefore the memset following the realloc() is invalid and may
constitute a use after free. The only case when it is valid is when ptr
== newptr, so you may as well use newptr there instead. Except you never
test if the allocation succeeded, so on failing allocation that would
crash.

Also, should realloc() decide to move a shrinking allocation, this code
would leave the first part of the memory in address space twice, and
once in a freed location, so clearing it is invalid, but an attacker may
still be able to read sensitive data.

I guess, for maximum performance, you would want a version of realloc()
that clears the old storage before freeing it if it does end up moving.
Except that is not part of the standard realloc() contract, and any
extension function would not be conducive to interposing libraries like
libefence.

The most portable options would be to always allocate new storage, or to
never even call realloc() when shrinking. Not the most performant things
in the world, but then, these extensions are meant for sensitive data.

Also, the manpage (I found this one: https://man.openbsd.org/recallocarray.3)
specifies an EINVAL error return. So, putting it all together, maybe
something like this?

void* recallocarray(void *ptr, size_t om, size_t m, size_t n) {
    if (!n || m > -1 / n) {
        errno = EINVAL;
        return 0;
    }
    size_t new_size = m * n;
    /* if ptr is null, oldnmemb is ignored, says the manpage */
    if (!ptr)
        om = 0;
    size_t old_size = om * n;
    if (new_size <= old_size) {
        memset((char*)ptr + new_size, 0, old_size - new_size);
        return ptr;
    }
    void *newptr = calloc(m, n);
    if (newptr && ptr) {
        memcpy(newptr, ptr, old_size);
        explicit_bzero(ptr, old_size);
        free(ptr);
    }
    return newptr; /* on failure, errno is already set from calloc(), right? */
}

As I said, always allocating new storage is not nice, but is the most
portable way I know how to be able to still manipulate the old storage
after the allocation of the new. Anything further would require sinking
hooks deeper into the allocator. Unless I missed something.

Ciao,
Markus

Re: [musl] [PATCH v3] implement recallocarray(3)

[Jens Gustedt]

[-- Attachment #1: Type: text/plain, Size: 2495 bytes --]

Hello,

on Sat,  1 Aug 2020 15:42:16 -0600 you (Ariadne Conill
<ariadne@dereferenced.org>) wrote:

> This OpenBSD extension is similar to reallocarray(3), but
> zero-initializes the new memory area.
> 
> This extension is placed in _BSD_SOURCE, like
> reallocarray(3).
> 
> Changes from v2:
> - drop overflow checking for old size
> 
> Changes from v1:
> - use realloc() instead of reallocarray()
> ---
>  include/stdlib.h           |  1 +
>  src/malloc/recallocarray.c | 27 +++++++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>  create mode 100644 src/malloc/recallocarray.c
> 
> diff --git a/include/stdlib.h b/include/stdlib.h
> index b54a051f..a0412ad4 100644
> --- a/include/stdlib.h
> +++ b/include/stdlib.h
> @@ -146,6 +146,7 @@ int clearenv(void);
>  #define WCOREDUMP(s) ((s) & 0x80)
>  #define WIFCONTINUED(s) ((s) == 0xffff)
>  void *reallocarray (void *, size_t, size_t);
> +void *recallocarray (void *, size_t, size_t, size_t);
>  #endif
>  
>  #ifdef _GNU_SOURCE
> diff --git a/src/malloc/recallocarray.c b/src/malloc/recallocarray.c
> new file mode 100644
> index 00000000..a7827604
> --- /dev/null
> +++ b/src/malloc/recallocarray.c
> @@ -0,0 +1,27 @@
> +#define _BSD_SOURCE
> +#include <errno.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +void *recallocarray(void *ptr, size_t om, size_t m, size_t n)
> +{
> +	void *newptr;
> +	size_t old_size = om * n, new_size;
> +
> +	if (n && m > -1 / n) {
> +		errno = ENOMEM;
> +		return 0;
> +	}
> +	new_size = m * n;
> +
> +	if (new_size <= old_size) {
> +		memset((char *) ptr + new_size, 0, old_size -
> new_size);
> +	}
> +
> +	newptr = realloc(ptr, m * n);

I think, this would better be

	newptr = realloc(ptr, new_size);

> +	if (new_size > old_size) {
> +		memset((char *) ptr + old_size, 0, new_size - old_size);
> +     }

Generally, if `realloc` succeeds, access to the object behind `ptr` is
invalid, even if `ptr == newptr`.

Also `newptr` may be null if `realloc` fails, so this should read

	if (newptr && new_size > old_size) {
		memset((char *)newptr + old_size, 0, new_size - old_size);
        }


Thanks
Jens

-- 
:: INRIA Nancy Grand Est ::: Camus ::::::: ICube/ICPS :::
:: ::::::::::::::: office Strasbourg : +33 368854536   ::
:: :::::::::::::::::::::: gsm France : +33 651400183   ::
:: ::::::::::::::: gsm international : +49 15737185122 ::
:: http://icube-icps.unistra.fr/index.php/Jens_Gustedt ::

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]