Re: [libvirt] [PATCH] Allocate buffer to hold xend response

Jim Fehlig wrote: ... > Subject: [PATCH] Allocate buffer to hold xend response > > There are cases when a response from xend can exceed 4096 bytes, in > which case anything beyond 4096 is ignored. This patch changes the > current fixed-size, stack-allocated buffer to a dynamically allocated > buffer based on Content-Length in HTTP header. > --- > src/xen/xend_internal.c | 80 ++++++++++++++++++++++++----------------------- > 1 files changed, 41 insertions(+), 39 deletions(-) > > diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c > index e763bad..0c1a738 100644 > --- a/src/xen/xend_internal.c > +++ b/src/xen/xend_internal.c > @@ -302,17 +302,19 @@ istartswith(const char *haystack, const char *needle) > * xend_req: > * @fd: the file descriptor > * @content: the buffer to store the content > - * @n_content: the size of the buffer > * > * Read the HTTP response from a Xen Daemon request. > + * If the response contains content, memory is allocated to > + * hold the content. > * > - * Returns the HTTP return code. > + * Returns the HTTP return code and @content is set to the > + * allocated memory containing HTTP content. > */ > static int > -xend_req(int fd, char *content, size_t n_content) > +xend_req(int fd, char **content) > { > char buffer[4096]; > - int content_length = -1; > + int content_length = 0; > int retcode = 0; > > while (sreads(fd, buffer, sizeof(buffer)) > 0) { > @@ -325,19 +327,17 @@ xend_req(int fd, char *content, size_t n_content) > retcode = atoi(buffer + 9); > } > > - if (content_length > -1) { > + if (content_length > 0) { > ssize_t ret; > > - if ((unsigned int) content_length > (n_content + 1)) > - content_length = n_content - 1; > + if (VIR_ALLOC_N(*content, content_length) < 0 ) { > + virReportOOMError(); > + return -1; > + } > > - ret = sread(fd, content, content_length); > + ret = sread(fd, *content, content_length); > if (ret < 0) > return -1; > - > - content[ret] = 0; > - } else { > - content[0] = 0; > } > Hi Jim, The above removes the code that guarantees the content buffer is NUL-terminated, yet I don't see where the requirement for NUL-termination has been dropped.

Even if the content payload we receive typically ends in a NUL, we cannot guarantee that (i.e., content may be corrupted or malicious), so we have to verify the assumption or add a NUL byte of our own. While your change does adjust the xend_req caller to handle NULL "content", here it will read beyond end of buffer when "*content" has no trailing NUL byte: virXendError(VIR_ERR_GET_FAILED, _("%d status from xen daemon: %s:%s"), - ret, path, content); + ret, path, + content ? *content: "NULL"); BTW, please add a space before the ":".

Also, now that we'll get the buffer length from an untrusted source, it would be good to ensure that it's not outrageously large before using it as the size of the buffer we allocate, assuming there is some reasonably low upper bound on the maximum payload size. That could save us from potential DoS abuse.