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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

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.

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


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

>                  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?

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

Thanks,


--
Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech