[PATCH v2 01/50] lib: Add memdup()

[Simon Glass]

Hi Rasmus,

On Mon, 10 May 2021 at 03:00, Rasmus Villemoes
<rasmus.villemoes at prevas.dk> wrote:
>
> On 06/05/2021 19.41, Simon Glass wrote:
> > Hi Pratyush,
> >
> > On Thu, 6 May 2021 at 10:07, Pratyush Yadav <p.yadav at ti.com> wrote:
> >>
> >> On 06/05/21 08:23AM, Simon Glass wrote:
> >>> Add a function to duplicate a memory region, a little like strdup().
> >>>
> >>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>> ---
> >>>
> >>> Changes in v2:
> >>> - Add a patch to introduce a memdup() function
> >>>
> >>>  include/linux/string.h | 13 +++++++++++++
> >>>  lib/string.c           | 13 +++++++++++++
> >>>  test/lib/string.c      | 32 ++++++++++++++++++++++++++++++++
> >>>  3 files changed, 58 insertions(+)
> >>>
> >>> diff --git a/include/linux/string.h b/include/linux/string.h
> >>> index dd255f21633..3169c93796e 100644
> >>> --- a/include/linux/string.h
> >>> +++ b/include/linux/string.h
> >>> @@ -129,6 +129,19 @@ extern void * memchr(const void *,int,__kernel_size_t);
> >>>  void *memchr_inv(const void *, int, size_t);
> >>>  #endif
> >>>
> >>> +/**
> >>> + * memdup() - allocate a buffer and copy in the contents
> >>> + *
> >>> + * Note that this returns a valid pointer even if @len is 0
> >>
> >> I'm uneducated about U-Boot's memory allocator. But I wonder how it
> >> returns a valid pointer even on 0 length allocations. What location does
> >> it point to? What are users expected to do with that pointer? They
> >> obviously can't read/write to it since it is supposed to be a 0 byte
> >> long allocation. If another positive length allocation happens before
> >> the said pointer is freed, will it point to the same memory location? If
> >> not, isn't the 0-length pointer actually at least a 1-length pointer?
> >
> > I think it is just a 0-length pointer and that the only thing you can
> > do with it is call free().
> >
> > I am certainly no expert on this sort of thing though. It seems that
> > some implementations return NULL for a zero size, some return a valid
> > pointer which can be passed to free().
>
> It's implementation-defined, which means that one cannot know for
> certain that a given malloc implementation won't return NULL for a
> request of 0 bytes. The linux kernel solved that problem by introducing

This function is for U-Boot board code. Note that the C-library
malloc() is used for tools and we don't run these tests using that, so
it is safe to assume that it is the U-Boot malloc() that is used in
all cases where this function is used.

> ZERO_SIZE_PTR which is basically just (void*)16L or something like that
> - that way callers don't have to write their "did the allocation
> succeed" test in the ugly
>
>   if (!p && size != 0)
>     error_out;
>
> way. Of course kfree() must then accept that in addition to NULL, but
> it's not really more expensive to have that early nop check be
>
>   if ((unsigned long)ptr <= 16)
>      return;
>
> instead of
>
>   if (!ptr)
>     return;
>
>
> "man malloc" says
>
> RETURN VALUE
>        The malloc() and calloc() functions return a pointer to the
> allocated memory, which is suitably aligned for any built-in type.  On
> error,  these  functions
>        return  NULL.   NULL may also be returned by a successful call to
> malloc() with a size of zero, or by a successful call to calloc() with
> nmemb or size equal
>        to zero.
>
>
> Anyway, I don't think this helper should be put in string.c - it needs
> to be in some C file that's easily compiled for both board, sandbox and
> host tools (for the latter probably via the "tiny one-line wrapper that
> just includes the whole real C file"). I see there's linux_string.c already.

I'm not a big fan of adding a function that can be used from tools as
well as U-Boot. Sandbox just uses the U-Boot libraries, so far as
possible. Only a few files even compile with the host C libraries
(e.g. os.c).

So if we want this functionality in the host tools, I suggest we add
it into the tools dir somewhere.

Regards,
SImon