On Thu, Sep 29, 2022 at 12:02:43AM -0400, Sean Anderson wrote:
> On 9/28/22 22:36, Simon Glass wrote:
> > Hi Sean,
> >
> > On Wed, 28 Sept 2022 at 15:50, Sean Anderson <sean...@gmail.com> wrote:
> > >
> > > On 9/28/22 17:06, Simon Glass wrote:
> > > > Hi Sean,
> > > >
> > > > On Wed, 28 Sept 2022 at 11:18, Sean Anderson <sean...@gmail.com> wrote:
> > > > >
> > > > > On 9/6/22 22:27, Simon Glass wrote:
> > > > > > It is helpful to test that out-of-memory checks work correctly in
> > > > > > code
> > > > > > that calls malloc().
> > > > > >
> > > > > > Add a simple way to force failure after a given number of malloc()
> > > > > > calls.
> > > > > >
> > > > > > Fix a header guard to avoid a build error on sandbox_vpl.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <s...@chromium.org>
> > > > > > ---
> > > > > >
> > > > > > (no changes since v1)
> > > > > >
> > > > > > arch/sandbox/include/asm/malloc.h | 1 +
> > > > > > common/dlmalloc.c | 19 +++++++++++++++++++
> > > > > > include/malloc.h | 12 ++++++++++++
> > > > > > test/test-main.c | 1 +
> > > > > > 4 files changed, 33 insertions(+)
> > > > > >
> > > > > > diff --git a/arch/sandbox/include/asm/malloc.h
> > > > > > b/arch/sandbox/include/asm/malloc.h
> > > > > > index a1467b5eadd..8aaaa9cb87b 100644
> > > > > > --- a/arch/sandbox/include/asm/malloc.h
> > > > > > +++ b/arch/sandbox/include/asm/malloc.h
> > > > > > @@ -6,6 +6,7 @@
> > > > > > */
> > > > > >
> > > > > > #ifndef __ASM_MALLOC_H
> > > > > > +#define __ASM_MALLOC_H
> > > > > >
> > > > > > void *malloc(size_t size);
> > > > > > void free(void *ptr);
> > > > > > diff --git a/common/dlmalloc.c b/common/dlmalloc.c
> > > > > > index f48cd2a333d..41c7230424c 100644
> > > > > > --- a/common/dlmalloc.c
> > > > > > +++ b/common/dlmalloc.c
> > > > > > @@ -596,6 +596,9 @@ ulong mem_malloc_start = 0;
> > > > > > ulong mem_malloc_end = 0;
> > > > > > ulong mem_malloc_brk = 0;
> > > > > >
> > > > > > +static bool malloc_testing; /* enable test mode */
> > > > > > +static int malloc_max_allocs; /* return NULL after this
> > > > > > many calls to malloc() */
> > > > > > +
> > > > > > void *sbrk(ptrdiff_t increment)
> > > > > > {
> > > > > > ulong old = mem_malloc_brk;
> > > > > > @@ -1307,6 +1310,11 @@ Void_t* mALLOc(bytes) size_t bytes;
> > > > > > return malloc_simple(bytes);
> > > > > > #endif
> > > > > >
> > > > > > + if (CONFIG_IS_ENABLED(UNIT_TEST) && malloc_testing) {
> > > > > > + if (--malloc_max_allocs < 0)
> > > > > > + return NULL;
> > > > > > + }
> > > > > > +
> > > > > > /* check if mem_malloc_init() was run */
> > > > > > if ((mem_malloc_start == 0) && (mem_malloc_end == 0)) {
> > > > > > /* not initialized yet */
> > > > > > @@ -2470,6 +2478,17 @@ int initf_malloc(void)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +void malloc_enable_testing(int max_allocs)
> > > > > > +{
> > > > > > + malloc_testing = true;
> > > > > > + malloc_max_allocs = max_allocs;
> > > > > > +}
> > > > > > +
> > > > > > +void malloc_disable_testing(void)
> > > > > > +{
> > > > > > + malloc_testing = false;
> > > > > > +}
> > > > > > +
> > > > > > /*
> > > > > >
> > > > > > History:
> > > > > > diff --git a/include/malloc.h b/include/malloc.h
> > > > > > index e8c8b254c0d..161ccbd1298 100644
> > > > > > --- a/include/malloc.h
> > > > > > +++ b/include/malloc.h
> > > > > > @@ -883,6 +883,18 @@ extern Void_t* sbrk();
> > > > > >
> > > > > > void malloc_simple_info(void);
> > > > > >
> > > > > > +/**
> > > > > > + * malloc_enable_testing() - Put malloc() into test mode
> > > > > > + *
> > > > > > + * This only works if UNIT_TESTING is enabled
> > > > > > + *
> > > > > > + * @max_allocs: return -ENOMEM after max_allocs calls to malloc()
> > > > > > + */
> > > > > > +void malloc_enable_testing(int max_allocs);
> > > > > > +
> > > > > > +/** malloc_disable_testing() - Put malloc() into normal mode */
> > > > > > +void malloc_disable_testing(void);
> > > > > > +
> > > > > > #if CONFIG_IS_ENABLED(SYS_MALLOC_SIMPLE)
> > > > > > #define malloc malloc_simple
> > > > > > #define realloc realloc_simple
> > > > > > diff --git a/test/test-main.c b/test/test-main.c
> > > > > > index c65cbd7c351..5b6b5ba5bbe 100644
> > > > > > --- a/test/test-main.c
> > > > > > +++ b/test/test-main.c
> > > > > > @@ -46,6 +46,7 @@ static int dm_test_pre_run(struct unit_test_state
> > > > > > *uts)
> > > > > > uts->force_fail_alloc = false;
> > > > > > uts->skip_post_probe = false;
> > > > > > gd->dm_root = NULL;
> > > > > > + malloc_disable_testing();
> > > > > > if (CONFIG_IS_ENABLED(UT_DM) &&
> > > > > > !CONFIG_IS_ENABLED(OF_PLATDATA))
> > > > > > memset(dm_testdrv_op_count, '\0',
> > > > > > sizeof(dm_testdrv_op_count));
> > > > > > arch_reset_for_test();
> > > > >
> > > > > Reviewed-by: Sean Anderson <sean...@gmail.com>
> > > > >
> > > > > but do we need to instrument rEALLOc too?
> > > >
> > > > We could, but I'd prefer to have a test which needs it first. Actually
> > > > realloc() calls malloc() in some cases, so we are already messing with
> > > > things here.
> > >
> > > Maybe it would be better to do this at the "top-level" instead. E.g.
> > > redefine the malloc() et al macro. That way we remove any double-counting.
> >
> > Yes, that makes sense. But again, I'd rather do this when we have a
> > use case for making realloc() fail.
>
> Fine by me.
>
> > BTW has dlmalloc been updated in recent years?
>
> Apparently there's a 2.8.6, last updated in 2012 [1].And I _think_ when I looked a year or two back there wasn't anything standing out as an important to try and update change in there. But I could be wrong and have missed something. -- Tom
signature.asc
Description: PGP signature
