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)")