[PATCH 01/49] Add support for an owned buffer

[Rasmus Villemoes]

On 04/05/2021 01.10, Simon Glass wrote:
> When passing a data buffer back from a function, it is not always clear
> who owns the buffer, i.e. who is responsible for freeing the memory used.
> An example of this is where multiple files are decompressed from the
> firmware image, using a temporary buffer for reading (since the
> compressed data has to live somewhere) and producing a temporary or
> permanent buffer with the resuilts.
> 
> Where the firmware image can be memory-mapped, as on x86, the compressed
> data does not need to be buffered, but the complexity of having a buffer
> which is either allocated or not, makes the code hard to understand.
> 
> Introduce a new 'abuf' which supports simple buffer operations:
> 
> - encapsulating a buffer and its size
> - either allocated with malloc() or not
> - able to be reliably freed if necessary
> - able to be converted to an allocated buffer if needed
> 
> This simple API makes it easier to deal with allocated and memory-mapped
> buffers.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
> Changes in v2:
> - Add new abuf_init_set() function
> 
>  include/abuf.h    | 148 ++++++++++++++++++++++
>  lib/Makefile      |   1 +
>  lib/abuf.c        | 103 ++++++++++++++++
>  test/lib/Makefile |   1 +
>  test/lib/abuf.c   | 303 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 556 insertions(+)
>  create mode 100644 include/abuf.h
>  create mode 100644 lib/abuf.c
>  create mode 100644 test/lib/abuf.c
> 
> diff --git a/include/abuf.h b/include/abuf.h
> new file mode 100644
> index 00000000000..3b8f78348dd
> --- /dev/null
> +++ b/include/abuf.h
> @@ -0,0 +1,148 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Handles a buffer that can be allocated and freed
> + *
> + * Copyright 2021 Google LLC
> + * Written by Simon Glass <sjg at chromium.org>
> + */
> +
> +#ifndef __ABUF_H
> +#define __ABUF_H
> +
> +#include <linux/types.h>
> +
> +/**
> + * struct abuf - buffer that can be allocated and freed
> + *
> + * This is useful for a block of data which may be allocated with malloc(), or
> + * not, so that it needs to be freed correctly when finished with.
> + *
> + * For now it has a very simple purpose.
> + *
> + * Using memset() to zero all fields is guaranteed to be equivalent to
> + * abuf_init().
> + *
> + * @data: Pointer to data
> + * @size: Size of data in bytes
> + * @alloced: true if allocated with malloc(), so must be freed after use
> + */
> +struct abuf {
> +	void *data;
> +	size_t size;
> +	bool alloced;
> +};
> +
> +static inline void *abuf_data(struct abuf *abuf)

const struct abuf *abuf?

> +{
> +	return abuf->data;
> +}
> +
> +static inline size_t abuf_size(struct abuf *abuf)

ditto

> +{
> +	return abuf->size;
> +}
> +
> +
> +bool abuf_realloc(struct abuf *abuf, size_t new_size)
> +{
> +	void *ptr;
> +
> +	if (!new_size) {
> +		/* easy case, just need to uninit, freeing any allocation */
> +		abuf_uninit(abuf);
> +	} else if (abuf->alloced) {
> +		/* currently allocated, so need to reallocate */
> +		ptr = realloc(abuf->data, new_size);
> +		if (!ptr)
> +			return false;
> +		abuf->data = ptr;
> +		abuf->size = new_size;
> +	} else if (new_size <= abuf->size) {
> +		/*
> +		 * not currently alloced and new size is no larger. Just update
> +		 * it. Data is lost off the end if new_size < abuf->size
> +		 */
> +		abuf->size = new_size;
> +	} else {
> +		/* not currently allocated and new size is larger. Alloc and
> +		 * copy in data. The new space is not inited.
> +		 */
> +		ptr = malloc(new_size);
> +		if (!ptr)
> +			return false;
> +		memcpy(ptr, abuf->data, min(new_size, abuf->size));

You know new_size > abuf->size, no need for the min().

> +		abuf->data = ptr;
> +		abuf->size = new_size;
> +		abuf->alloced = true;
> +	}
> +
> +	return true;
> +}

I think this would be easier to read if the branches did an early
"return true;" when the case has been handled.

> +
> +void *abuf_uninit_move(struct abuf *abuf, size_t *sizep)
> +{
> +	void *ptr;
> +
> +	if (sizep)
> +		*sizep = abuf->size;
> +	if (!abuf->size)
> +		return NULL;
> +	if (abuf->alloced) {
> +		ptr = abuf->data;
> +	} else {
> +		ptr = malloc(abuf->size);
> +		if (!ptr)
> +			return NULL;
> +		memcpy(ptr, abuf->data, abuf->size);

Don't we have a memdup() function? Hm, no, only a kmemdup(). Might be
worth introducing at some point, quick grepping shows quite a few places
that could use that.

> +	}
> +	/* Clear everything out so there is no record of the data */
> +	abuf_init(abuf);
> +
> +	return ptr;
> +}
> +
> +void abuf_init_set(struct abuf *abuf, void *data, size_t size)
> +{
> +	abuf_init(abuf);
> +	abuf_set(abuf, data, size);
> +}
> +
> +void abuf_uninit(struct abuf *abuf)
> +{
> +	if (abuf->alloced)
> +		free(abuf->data);
> +	abuf_init(abuf);
> +}
> +
> +void abuf_init(struct abuf *abuf)
> +{
> +	abuf->data = NULL;
> +	abuf->size = 0;
> +	abuf->alloced = false;
> +}
> diff --git a/test/lib/Makefile b/test/lib/Makefile
> index aa2e66bc7f4..02e7cda532d 100644
> --- a/test/lib/Makefile
> +++ b/test/lib/Makefile
> @@ -3,6 +3,7 @@
>  # (C) Copyright 2018
>  # Mario Six, Guntermann & Drunck GmbH, mario.six at gdsys.cc
>  obj-y += cmd_ut_lib.o
> +obj-y += abuf.o
>  obj-$(CONFIG_EFI_LOADER) += efi_device_path.o
>  obj-$(CONFIG_EFI_SECURE_BOOT) += efi_image_region.o
>  obj-y += hexdump.o
> diff --git a/test/lib/abuf.c b/test/lib/abuf.c
> new file mode 100644
> index 00000000000..45c5c131b34
> --- /dev/null
> +++ b/test/lib/abuf.c
> @@ -0,0 +1,303 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2021 Google LLC
> + * Written by Simon Glass <sjg at chromium.org>
> + */
> +
> +#include <common.h>
> +#include <abuf.h>
> +#include <mapmem.h>
> +#include <test/lib.h>
> +#include <test/test.h>
> +#include <test/ut.h>
> +
> +static char test_data[] = "1234";
> +#define TEST_DATA_LEN	sizeof(test_data)
> +
> +/* Test abuf_set() */
> +static int lib_test_abuf_set(struct unit_test_state *uts)
> +{
> +	struct abuf buf;
> +	ulong start;
> +
> +	start = ut_check_free();
> +
> +	abuf_init(&buf);
> +	abuf_set(&buf, test_data, TEST_DATA_LEN);
> +	ut_asserteq_ptr(test_data, buf.data);
> +	ut_asserteq(TEST_DATA_LEN, buf.size);
> +	ut_asserteq(false, buf.alloced);
> +
> +	/* Force it to allocate */
> +	ut_asserteq(true, abuf_realloc(&buf, TEST_DATA_LEN + 1));
> +	ut_assertnonnull(buf.data);
> +	ut_asserteq(TEST_DATA_LEN + 1, buf.size);
> +	ut_asserteq(true, buf.alloced);
> +
> +	/* Now set it again, to force it to free */
> +	abuf_set(&buf, test_data, TEST_DATA_LEN);
> +	ut_asserteq_ptr(test_data, buf.data);
> +	ut_asserteq(TEST_DATA_LEN, buf.size);
> +	ut_asserteq(false, buf.alloced);
> +
> +	/* Check for memory leaks */
> +	ut_assertok(ut_check_delta(start));
> +
> +	return 0;
> +}
> +LIB_TEST(lib_test_abuf_set, 0);
> +
> +/* Test abuf_map_sysmem() */
> +static int lib_test_abuf_map_sysmem(struct unit_test_state *uts)
> +{
> +	struct abuf buf;
> +	ulong addr;
> +
> +	abuf_init(&buf);
> +	addr = 0x100;
> +	abuf_map_sysmem(&buf, addr, TEST_DATA_LEN);
> +
> +	ut_asserteq_ptr(map_sysmem(0x100, 0), buf.data);
> +	ut_asserteq(TEST_DATA_LEN, buf.size);
> +	ut_asserteq(false, buf.alloced);
> +
> +	return 0;
> +}
> +LIB_TEST(lib_test_abuf_map_sysmem, 0);
> +
> +/* Test abuf_realloc() */
> +static int lib_test_abuf_realloc(struct unit_test_state *uts)
> +{
> +	struct abuf buf;
> +	ulong start;
> +	void *ptr;
> +
> +	/* TODO: crashes on sandbox */
> +	return 0;
> +

Eh?

> +	start = ut_check_free();
> +
> +	abuf_init(&buf);
> +
> +	/* Allocate an empty buffer */
> +	ut_asserteq(true, abuf_realloc(&buf, 0));
> +	ut_assertnull(buf.data);
> +	ut_asserteq(0, buf.size);
> +	ut_asserteq(false, buf.alloced);
> +
> +	/* Allocate a non-empty abuf */
> +	ut_asserteq(true, abuf_realloc(&buf, TEST_DATA_LEN));
> +	ut_assertnonnull(buf.data);
> +	ut_asserteq(TEST_DATA_LEN, buf.size);
> +	ut_asserteq(true, buf.alloced);
> +	ptr = buf.data;
> +
> +	/* Make it smaller; the pointer should remain the same  */
> +	ut_asserteq(true, abuf_realloc(&buf, TEST_DATA_LEN - 1));
> +	ut_asserteq(TEST_DATA_LEN - 1, buf.size);
> +	ut_asserteq(true, buf.alloced);
> +	ut_asserteq_ptr(ptr, buf.data);

Perhaps this one? In the alloc'ed case, you always do a realloc()
whether or not the new size is larger. realloc() is free to return the
same pointer, or actually trim the excess off, or do a
malloc()+memcpy()+free() under the hood. realloc() isn't even required
to succeed if the new size is smaller than the old. So I don't see how
you can assert anything about how realloc() will behave.

If you change the abuf_realloc() to be a no-op (except for updating
->size) for the case of new_size < abuf->size, then yes, you can of
course check that abuf behaves that way. But as soon as realloc() is
invoked, all bets are off.

> +	/* Make it larger, forcing reallocation */
> +	ut_asserteq(true, abuf_realloc(&buf, 0x1000));
> +	ut_assert(buf.data != ptr);

And this one is similarly flawed - if malloc() originally handed out a
buffer that is actually (much) larger than you requested, only the
malloc implementation knows, but that can very well mean that a later
realloc() does not need to actually do a new allocation - or perhaps,
the original allocation wasn't much too big, but it turns out that the
following chunk of memory is currently on the free list, so the existing
allocation can be expanded in-place.

> +	ut_asserteq(0x1000, buf.size);
> +	ut_asserteq(true, buf.alloced);
> +
> +	/* Free it */
> +	ut_asserteq(true, abuf_realloc(&buf, 0));
> +	ut_assertnull(buf.data);
> +	ut_asserteq(0, buf.size);
> +	ut_asserteq(false, buf.alloced);
> +
> +	/* Check for memory leaks */
> +	ut_assertok(ut_check_delta(start));
> +
> +	return 0;
> +}
> +LIB_TEST(lib_test_abuf_realloc, 0);
> +
> +/* Test handling of buffers that are too large */
> +static int lib_test_abuf_large(struct unit_test_state *uts)
> +{
> +	struct abuf buf;
> +	ulong start;
> +	size_t size;
> +	int delta;
> +	void *ptr;
> +
> +	/*
> +	 * This crashes at present due to trying to allocate more memory than
> +	 * available, which breaks something on sandbox.
> +	 */
> +	return 0;
> +
> +	start = ut_check_free();
> +
> +	/* Try an impossible size */
> +	abuf_init(&buf);
> +	ut_asserteq(false, abuf_realloc(&buf, CONFIG_SYS_MALLOC_LEN));
> +	ut_assertnull(buf.data);
> +	ut_asserteq(0, buf.size);
> +	ut_asserteq(false, buf.alloced);
> +
> +	abuf_uninit(&buf);
> +	ut_assertnull(buf.data);
> +	ut_asserteq(0, buf.size);
> +	ut_asserteq(false, buf.alloced);
> +
> +	/* Start with a normal size then try to increase it, to check realloc */
> +	ut_asserteq(true, abuf_realloc(&buf, TEST_DATA_LEN));
> +	ut_assertnonnull(buf.data);
> +	ut_asserteq(TEST_DATA_LEN, buf.size);
> +	ut_asserteq(true, buf.alloced);
> +	ptr = buf.data;
> +	delta = ut_check_delta(start);
> +	ut_assert(delta > 0);
> +
> +	/* try to increase it */
> +	ut_asserteq(false, abuf_realloc(&buf, CONFIG_SYS_MALLOC_LEN));
> +	ut_asserteq_ptr(ptr, buf.data);
> +	ut_asserteq(TEST_DATA_LEN, buf.size);
> +	ut_asserteq(true, buf.alloced);
> +	ut_asserteq(delta, ut_check_delta(start));
> +
> +	/* Check for memory leaks */
> +	abuf_uninit(&buf);
> +	ut_assertok(ut_check_delta(start));
> +
> +	/* Start with a huge unallocated buf and try to move it */
> +	abuf_init(&buf);
> +	abuf_map_sysmem(&buf, 0, CONFIG_SYS_MALLOC_LEN);
> +	ut_asserteq(CONFIG_SYS_MALLOC_LEN, buf.size);
> +	ut_asserteq(false, buf.alloced);
> +	ut_assertnull(abuf_uninit_move(&buf, &size));
> +
> +	/* Check for memory leaks */
> +	abuf_uninit(&buf);
> +	ut_assertok(ut_check_delta(start));
> +
> +	return 0;
> +}
> +LIB_TEST(lib_test_abuf_large, 0);
> +
> +/* Test abuf_uninit_move() */
> +static int lib_test_abuf_uninit_move(struct unit_test_state *uts)
> +{
> +	void *ptr, *orig_ptr;
> +	struct abuf buf;
> +	size_t size;
> +	ulong start;
> +	int delta;
> +
> +	start = ut_check_free();
> +
> +	/* Move an empty buffer */
> +	abuf_init(&buf);
> +	ut_assertnull(abuf_uninit_move(&buf, &size));
> +	ut_asserteq(0, size);
> +	ut_assertnull(abuf_uninit_move(&buf, NULL));
> +
> +	/* Move an unallocated buffer */
> +	abuf_set(&buf, test_data, TEST_DATA_LEN);

Reading this made me think of something that I think is missing from the
API: Perhaps I have a chunk of memory that I have malloc'ed myself, and
now I want to use an abuf to manage it. How do I gift that to abuf? IOW,
perhaps an abuf_set_alloced(), or if it's better to think of it as a
companion to abuf_uninit_move, perhaps abuf_init_move()?

Rasmus