Re: [PATCH 3/6] tools/libxl: Allow adding larger amounts of prefixdata to datacopier

[Ian Campbell <ian.campbell@citrix.com>]

On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Previously, adding more than 1000 bytes of data would cause a segfault.
> Now, the maximum amount of data that can be added is limited by maxsz.

http://lists.xen.org/archives/html/xen-devel/2014-09/msg01806.html:
        struct libxl__datacopier_buf contains a fixed size 1000 byte
        statically allocated buffer so adding > 1000 bytes of data would
        cause it to overrun the buffer and overwrite other memory.
http://lists.xen.org/archives/html/xen-devel/2014-09/msg01813.html
        Yes, this should be the main point of the commit log though.

The commit log should mention that the current code overruns a static
1000 byte buffer and fixes it by allocating and chaining as many buffers
as are required for the amount of data.

> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl_aoutils.c |   14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/libxl_aoutils.c b/tools/libxl/libxl_aoutils.c
> index 3e0c0ae..6882ca3 100644
> --- a/tools/libxl/libxl_aoutils.c
> +++ b/tools/libxl/libxl_aoutils.c
> @@ -160,6 +160,8 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
>  {
>      EGC_GC;
>      libxl__datacopier_buf *buf;
> +    const uint8_t *ptr;
> +
>      /*
>       * It is safe for this to be called immediately after _start, as
>       * is documented in the public comment.  _start's caller must have
> @@ -170,12 +172,14 @@ void libxl__datacopier_prefixdata(libxl__egc *egc, libxl__datacopier_state *dc,
>  
>      assert(len < dc->maxsz - dc->used);
>  
> -    buf = libxl__zalloc(NOGC, sizeof(*buf));
> -    buf->used = len;
> -    memcpy(buf->buf, data, len);
> +    for (ptr = data; len; len -= buf->used, ptr += buf->used) {
> +        buf = libxl__malloc(NOGC, sizeof(*buf));
> +        buf->used = min(len, sizeof(buf->buf));
> +        memcpy(buf->buf, ptr, buf->used);
>  
> -    dc->used += len;
> -    LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> +        dc->used += buf->used;
> +        LIBXL_TAILQ_INSERT_TAIL(&dc->bufs, buf, entry);
> +    }
>  }
>  
>  static int datacopier_pollhup_handled(libxl__egc *egc,