[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 <jfehlig linux-ypgk site>
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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]