Re: [PATCH v9 4/4] libs/guest: use Valgrind to detect various buffer overflows

On Wed, 15 Jul 2026 at 14:32, Anthony PERARD <[email protected]> wrote:
>
> On Mon, Jul 13, 2026 at 09:48:02PM +0100, Frediano Ziglio wrote:
> > --- a/tools/libs/ctrl/xc_private.h
> > +++ b/tools/libs/ctrl/xc_private.h
> > @@ -45,8 +45,16 @@
> >  #if defined(HAVE_VALGRIND_MEMCHECK_H) && !defined(NDEBUG) && !defined(__MINIOS__)
> >  /* Compile in Valgrind client requests? */
> >  #include <valgrind/memcheck.h>
> > +#define MEM_NOACCESS_BUFFER(name, size) uint8_t name[size];
> > +#define MEM_NOACCESS_INIT(field) \
> > +    VALGRIND_MAKE_MEM_NOACCESS(field, sizeof(field))
> > +#define MEM_UNDEFINED_INIT(field) \
> > +    VALGRIND_MAKE_MEM_UNDEFINED(field, sizeof(field))
> >  #else
> >  #define VALGRIND_MAKE_MEM_UNDEFINED(addr, len) /* addr, len */
> > +#define MEM_NOACCESS_BUFFER(name, size)
> > +#define MEM_NOACCESS_INIT(field) do {} while(0)
> > +#define MEM_UNDEFINED_INIT(field) do {} while(0)
>
> Why the _INIT suffix in the macros? It looks like something is
> initialised, but that's not the case.
>

It initializes the memory to be not accessible.

> We could follow the valgrind naming and do, while using something
> different than "mem" to say we don't need a size:
> make_buffer_noaccess and make_buffer_undefined
>
> Or
> mark_buffer_as_noaccess
> mark_buffer_as_undefined
>

It makes sense, but I wanted to be able to remove the buffers entirely
if debug is not enabled.

>
> >  #endif
> >
> >  #if defined(__MINIOS__)
> > diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
> > index c07c6db59e..d3fc7f363e 100644
> > --- a/tools/libs/guest/xg_sr_common.h
> > +++ b/tools/libs/guest/xg_sr_common.h
> > @@ -245,13 +245,21 @@ struct xc_sr_context
> >              xc_hypercall_buffer_t dirty_bitmap_hbuf;
> >              struct xc_sr_context_save_buffers
> >              {
> > +                MEM_NOACCESS_BUFFER(na0, 16);
>
> This first redzone buffer and the last (na7) one looks unnecessary, as
> they are before the beginning of the buffer, and after the end.
>

Before for buffer underflow, after for buffer overflow, but I suppose
you mean that Valgrind/ASAN already add them anyway so it's not really
necessary.
Yes, I'll remove.

> >                  xen_pfn_t batch_pfns[MAX_BATCH_SIZE];
> > +                MEM_NOACCESS_BUFFER(na1, 16);
> >                  xen_pfn_t mfns[MAX_BATCH_SIZE];
> > +                MEM_NOACCESS_BUFFER(na2, 16);
> >                  xen_pfn_t types[MAX_BATCH_SIZE];
> > +                MEM_NOACCESS_BUFFER(na3, 16);
> >                  void *local_pages[MAX_BATCH_SIZE];
> > +                MEM_NOACCESS_BUFFER(na4, 16);
> >                  struct iovec iov[MAX_BATCH_SIZE + 2]; /* Headers + data. */
> > +                MEM_NOACCESS_BUFFER(na5, 16);
> >                  uint64_t rec_pfns[MAX_BATCH_SIZE];
> > +                MEM_NOACCESS_BUFFER(na6, 16);
> >                  int errors[MAX_BATCH_SIZE];
> > +                MEM_NOACCESS_BUFFER(na7, 16);
> >              } *buffers;
> >          } save;
> >
> > diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
> > index 6a77e33a47..25561e369f 100644
> > --- a/tools/libs/guest/xg_sr_save.c
> > +++ b/tools/libs/guest/xg_sr_save.c
> > @@ -123,6 +123,11 @@ static int write_batch(struct xc_sr_context *ctx)
> >      assert(nr_pfns != 0);
> >      assert(nr_pfns <= MAX_BATCH_SIZE);
> >
> > +    MEM_UNDEFINED_INIT(ctx->save.buffers->mfns);
> > +    MEM_UNDEFINED_INIT(ctx->save.buffers->types);
> > +    MEM_UNDEFINED_INIT(ctx->save.buffers->iov);
> > +    MEM_UNDEFINED_INIT(ctx->save.buffers->rec_pfns);
>
> Why is errors not also marked as undefined?
>

I suppose a rebase mistake, later in the series errors field is removed.

> To bad we can't really test this patch, beside check that it build.
>

Indeed, I realize Valgrind support is not that updated at the moment.

I'll add ASAN support as Andrew suggested.

> Thanks,
>
>
> --
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech

Frediano