Re: [PATCH] Allocate buffer to hold xend response

[Jim Meyering <jim@xxxxxxxxxxxx>]

Jim Fehlig wrote:
> Eric Blake wrote:
>>> 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.
>>
>> Looking forward to the followup, and thanks for Jim for catching
>> something I missed in my ACK (even if we didn't catch it in time).
>
> Patch that includes Jim's suggestions ...
...
> Subject: [PATCH] Fixes for commit 211dd1e9
>
> Fixes for issues in commit 211dd1e9 noted by by Jim Meyering.
>
> 1. Allocate content buffer of size content_length + 1 to ensure
>    NUL-termination.
> 2. Limit content buffer size to 64k
> 3. Fix whitespace issue
> ---
>  src/xen/xend_internal.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
> index 0c1a738..3b9da6b 100644
> --- a/src/xen/xend_internal.c
> +++ b/src/xen/xend_internal.c
> @@ -68,6 +68,7 @@
>  # define XEND_CONFIG_MIN_VERS_PVFB_NEWCONF 3
>  #endif
>
> +#define XEND_RCV_BUF_MAX_LEN 65536
>
>  #ifndef PROXY
>  static int
> @@ -330,7 +331,16 @@ xend_req(int fd, char **content)
>      if (content_length > 0) {
>          ssize_t ret;

Hi Jim,

Thanks for adding that.
This part looks fine.

> -        if (VIR_ALLOC_N(*content, content_length) < 0 ) {
> +        if (content_length > XEND_RCV_BUF_MAX_LEN) {
> +            virXendError(VIR_ERR_INTERNAL_ERROR,
> +                         _("Xend returned HTTP Content-Length of %d, "
> +                           "which exceeds maximum of %d"),
> +                         content_length,
> +                         XEND_RCV_BUF_MAX_LEN);
> +            return -1;
> +        }
> +

This is subtle enough that a comment might avoid trouble later.

/* Allocate one byte beyond the end of the largest buffer we will read.
   Combined with the fact that VIR_ALLOC_N zeros the returned buffer,
   this guarantees that "content" will always be NUL-terminated.  */

> +        if (VIR_ALLOC_N(*content, content_length + 1) < 0 ) {
>              virReportOOMError();
>              return -1;
>          }
> @@ -380,7 +390,7 @@ xend_get(virConnectPtr xend, const char *path,
>          virXendError(VIR_ERR_GET_FAILED,
>                       _("%d status from xen daemon: %s:%s"),
>                       ret, path,
> -                     content ? *content: "NULL");
> +                     content ? *content : "NULL");

Oh!  I've just noticed that testing "content" is wrong,
since it will always be non-NULL.  BTW, the parameter should
be marked as such via "ATTRIBUTE_NONNULL (3)".

The test should examine "*content", i.e.,

                        *content ? *content : "NULL");

Then I remembered the NULLSTR macro.
You can replace the above with this:

                        NULLSTR(*content));


FYI, here's its definition:

    $ git grep -A2 ine.NULLSTR
    src/internal.h:# define NULLSTR(s) \
    src/internal.h-    ((void)verify_true(sizeof *(s) == sizeof (char)), \
    src/internal.h-     (s) ? (s) : "(null)")

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list