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

[Jim Fehlig]

Jim Meyering wrote:
> 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.

I removed that since VIR_ALLOC_N fills the memory with zeros.  Correct?

>   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 ":".
>   

Ok.

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

Good point.  Suggestions on the limit?

BTW, I pushed the patch after Eric's ACK.  I'll role another to address 
these issues, once I get confirmation on the NUL-termination.

Thanks,
Jim