From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH 3/6] tools/libxl: Allow adding larger amounts of prefixdata to datacopier Date: Fri, 20 Feb 2015 10:32:52 +0000 Message-ID: <1424428372.30924.174.camel@citrix.com> References: <1424277263-27745-1-git-send-email-andrew.cooper3@citrix.com> <1424277263-27745-4-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1424277263-27745-4-git-send-email-andrew.cooper3@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper Cc: Ross Lagerwall , Ian Jackson , Wei Liu , Xen-devel List-Id: xen-devel@lists.xenproject.org On Wed, 2015-02-18 at 16:34 +0000, Andrew Cooper wrote: > From: Ross Lagerwall > > 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 > Signed-off-by: Andrew Cooper > CC: Ian Campbell > CC: Ian Jackson > CC: Wei Liu > --- > 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,