[libvirt] [PATCH] Allocate buffer to hold xend response
Jim Meyering
jim at meyering.net
Fri Jun 4 07:47:45 UTC 2010
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)")
More information about the libvir-list
mailing list