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

[Simon Glass]

Hi Rasmus,

On Mon, 3 May 2021 at 23:59, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> 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
> >
[..]

> > +/* 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?

I'll expand the comment. It is unrelated to this patch, buft there is
a problem in sandbox with memory allocation.

>
> > +     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.

Yes that is true, but this matches the behaviour of realloc() in
U-Boot. I will add a comment.

>
> > +     /* 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.

Same here. Going to the next power of 2 causes a realloc.

>
> > +     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()?

OK, thanks, good idea.. Will add.

Regards,
Simon