| Message ID | 20210506082420.v2.1.I1d417387eb1e7273b536017f4a8920fc4e2369a9@changeid |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | image: Reduce #ifdefs and ad-hoc defines in image code | expand |
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@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? > + * > + * @src: data to copy in > + * @len: number of bytes to copy > + * @return allocated buffer with the copied contents, or NULL if not enough > + * memory is available > + * > + */ > +char *memdup(const void *src, size_t len); > + > unsigned long ustrtoul(const char *cp, char **endp, unsigned int base); > unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base); > > diff --git a/lib/string.c b/lib/string.c > index a0cff8fe88e..1be61ee0499 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -658,6 +658,19 @@ void * memscan(void * addr, int c, size_t size) > } > #endif > > +char *memdup(const void *src, size_t len) > +{ > + char *p; > + > + p = malloc(len); > + if (!p) > + return NULL; > + > + memcpy(p, src, len); > + > + return p; > +} > + > #ifndef __HAVE_ARCH_STRSTR > /** > * strstr - Find the first substring in a %NUL terminated string [...]
Hi Pratyush, On Thu, 6 May 2021 at 10:07, Pratyush Yadav <p.yadav@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@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(). Of course, U-Boot lets you pass NULL to free() anyway. Regards, Simon
On 5/6/21 1:41 PM, Simon Glass wrote: > Hi Pratyush, > > On Thu, 6 May 2021 at 10:07, Pratyush Yadav <p.yadav@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@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(). Of course, U-Boot lets you pass > NULL to free() anyway. dlmalloc has a minimum allocation size of 2*sizeof(void *) (e.g. MINSIZE - 2*SIZE_SZ). If you request less than this, you will get an allocation of this size. This is the same as other requests, which are rounded up the the nearest multiple of MALLOC_ALIGNMENT. Of course, malloc_simple will actually give you a zero-byte allocation, so don't rely on being able to store anything there ;) --Sean
On 06/05/2021 19.41, Simon Glass wrote: > Hi Pratyush, > > On Thu, 6 May 2021 at 10:07, Pratyush Yadav <p.yadav@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@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 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. Rasmus
On 5/10/21 11:00 AM, Rasmus Villemoes wrote: > On 06/05/2021 19.41, Simon Glass wrote: >> Hi Pratyush, >> >> On Thu, 6 May 2021 at 10:07, Pratyush Yadav <p.yadav@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@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 > 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. > > Rasmus > Our malloc() implementation allocates space for metadata of type struct malloc_chunk even if the argument is zero. This metadata is used to check that a call to free() refers to a valid pointer. I don't see a need to change this behavior. Best regards Heinrich
Hi Rasmus, On Mon, 10 May 2021 at 03:00, Rasmus Villemoes <rasmus.villemoes@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@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@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
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 + * + * @src: data to copy in + * @len: number of bytes to copy + * @return allocated buffer with the copied contents, or NULL if not enough + * memory is available + * + */ +char *memdup(const void *src, size_t len); + unsigned long ustrtoul(const char *cp, char **endp, unsigned int base); unsigned long long ustrtoull(const char *cp, char **endp, unsigned int base); diff --git a/lib/string.c b/lib/string.c index a0cff8fe88e..1be61ee0499 100644 --- a/lib/string.c +++ b/lib/string.c @@ -658,6 +658,19 @@ void * memscan(void * addr, int c, size_t size) } #endif +char *memdup(const void *src, size_t len) +{ + char *p; + + p = malloc(len); + if (!p) + return NULL; + + memcpy(p, src, len); + + return p; +} + #ifndef __HAVE_ARCH_STRSTR /** * strstr - Find the first substring in a %NUL terminated string diff --git a/test/lib/string.c b/test/lib/string.c index 64234bef36c..5dcf4d6db00 100644 --- a/test/lib/string.c +++ b/test/lib/string.c @@ -23,6 +23,8 @@ /* Allow for copying up to 32 bytes */ #define BUFLEN (SWEEP + 33) +#define TEST_STR "hello" + /** * init_buffer() - initialize buffer * @@ -193,3 +195,33 @@ static int lib_memmove(struct unit_test_state *uts) } LIB_TEST(lib_memmove, 0); + +/** lib_memdup() - unit test for memdup() */ +static int lib_memdup(struct unit_test_state *uts) +{ + char buf[BUFLEN]; + size_t len; + char *p, *q; + + /* Zero size should do nothing */ + p = memdup(NULL, 0); + ut_assertnonnull(p); + free(p); + + p = memdup(buf, 0); + ut_assertnonnull(p); + free(p); + + strcpy(buf, TEST_STR); + len = sizeof(TEST_STR); + p = memdup(buf, len); + ut_asserteq_mem(p, buf, len); + + q = memdup(p, len); + ut_asserteq_mem(q, buf, len); + free(q); + free(p); + + return 0; +} +LIB_TEST(lib_memdup, 0);
Add a function to duplicate a memory region, a little like strdup(). Signed-off-by: Simon Glass <sjg@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(+)