Jim Meyering wrote:
>> - 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. */
>
Good idea. I've added your wording verbatim.
>
>> + 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)".
>
I added ATTRIBUTE_NONNULL to xend_req() as well.
> 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)")
>
Cool, that's handy. Updated patch attached.
Regards,
Jim
>From 738d6aebf5b0e2b9569095fec9d8dee669694215 Mon Sep 17 00:00:00 2001
From: Jim Fehlig <[email protected]>
Date: Fri, 4 Jun 2010 10:04:03 -0600
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
V2:
- Add comment to clarify allocation of content buffer
- Add ATTRIBUTE_NONNULL where appropriate
- User NULLSTR macro
---
src/xen/xend_internal.c | 22 +++++++++++++++++-----
1 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c
index 0c1a738..51cad92 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
@@ -310,7 +311,7 @@ istartswith(const char *haystack, const char *needle)
* Returns the HTTP return code and @content is set to the
* allocated memory containing HTTP content.
*/
-static int
+static int ATTRIBUTE_NONNULL (2)
xend_req(int fd, char **content)
{
char buffer[4096];
@@ -330,7 +331,19 @@ xend_req(int fd, char **content)
if (content_length > 0) {
ssize_t ret;
- 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;
+ }
+
+ /* 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;
}
@@ -353,7 +366,7 @@ xend_req(int fd, char **content)
*
* Returns the HTTP return code or -1 in case or error.
*/
-static int
+static int ATTRIBUTE_NONNULL(3)
xend_get(virConnectPtr xend, const char *path,
char **content)
{
@@ -379,8 +392,7 @@ xend_get(virConnectPtr xend, const char *path,
((ret != 404) || (!STRPREFIX(path, "/xend/domain/")))) {
virXendError(VIR_ERR_GET_FAILED,
_("%d status from xen daemon: %s:%s"),
- ret, path,
- content ? *content: "NULL");
+ ret, path, NULLSTR(*content));
}
return ret;
--
1.6.0.2
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
