Cleanedup test cases (they had some valgrind warnings).

Rewrite response parsing.
Refactored server list to no longer use null serer (this is the first of a
series of cleanups to this code).

diff --git a/ChangeLog b/ChangeLog
index a77151a5017c3da6ca17f25d19e2cf456474ac48..15316a2076f2299af6936382d8f3f1ae0bddcb59 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -4,6 +4,7 @@
     value was returned.
   * Bug fixed in memcached_connect() which would cause servers that 
     did not need to be enabled to be enabled (performance issue).
+  * Rewrote bounds checking code for get calls
 
 0.10 Tue Nov 20 23:22:31 PST 2007
   * Added append binary test.

diff --git a/include/memcached.h b/include/memcached.h
index 48790ce8e28e4921b65015ffc1b94862a25e5655..122b6f9790b321ca3d30dc9fc23b94ef547d0977 100644 (file)
--- a/include/memcached.h
+++ b/include/memcached.h
@@ -120,6 +120,7 @@ struct memcached_server_st {
   uint8_t major_version;
   uint8_t minor_version;
   uint8_t micro_version;
+  uint16_t count;
 };
 
 struct memcached_stat_st {

diff --git a/lib/memcached_get.c b/lib/memcached_get.c
index c5156439f9dc09166d77f3e414972faa27b1dcfd..98e24d9fbf938ba3c72a04eaa6ec0c783bad7c2d 100644 (file)
--- a/lib/memcached_get.c
+++ b/lib/memcached_get.c
@@ -34,7 +34,7 @@ static memcached_return memcached_value_fetch(memcached_st *ptr, char *key, size
     {
       *key_length= 0;
 
-      for (; end_ptr > string_ptr && *string_ptr != ' '; string_ptr++)
+      for (; isalnum(*string_ptr); string_ptr++)
       {
         *key= *string_ptr;
         key++;
@@ -42,7 +42,7 @@ static memcached_return memcached_value_fetch(memcached_st *ptr, char *key, size
       }
     }
     else /* Skip characters */
-      for (; end_ptr > string_ptr && *string_ptr != ' '; string_ptr++);
+      for (; isalnum(*string_ptr); string_ptr++);
 
     if (end_ptr == string_ptr)
         goto read_error;
@@ -51,7 +51,7 @@ static memcached_return memcached_value_fetch(memcached_st *ptr, char *key, size
     string_ptr++;
     if (end_ptr == string_ptr)
         goto read_error;
-    for (next_ptr= string_ptr; end_ptr > string_ptr && *string_ptr != ' '; string_ptr++);
+    for (next_ptr= string_ptr; isdigit(*string_ptr); string_ptr++);
     *flags= (uint16_t)strtol(next_ptr, &string_ptr, 10);
 
     if (end_ptr == string_ptr)
@@ -62,7 +62,7 @@ static memcached_return memcached_value_fetch(memcached_st *ptr, char *key, size
     if (end_ptr == string_ptr)
         goto read_error;
 
-    for (next_ptr= string_ptr; end_ptr > string_ptr && *string_ptr != ' '; string_ptr++);
+    for (next_ptr= string_ptr; isdigit(*string_ptr); string_ptr++);
     value_length= (size_t)strtoll(next_ptr, &string_ptr, 10);
 
     if (end_ptr == string_ptr)
@@ -77,7 +77,7 @@ static memcached_return memcached_value_fetch(memcached_st *ptr, char *key, size
     else
     {
       string_ptr++;
-      for (next_ptr= string_ptr; end_ptr > string_ptr && *string_ptr != ' '; string_ptr++);
+      for (next_ptr= string_ptr; isdigit(*string_ptr); string_ptr++);
       if (cas)
         *cas= (size_t)strtoll(next_ptr, &string_ptr, 10);
     }
@@ -209,15 +209,14 @@ char *memcached_get(memcached_st *ptr, char *key, size_t key_length,
 
   LIBMEMCACHED_MEMCACHED_GET_END();
 
-
-  return  memcached_string_c_copy(result_buffer);
+  return memcached_string_c_copy(result_buffer);
 
 error:
   *value_length= 0;
 
   LIBMEMCACHED_MEMCACHED_GET_END();
 
-    return NULL;
+  return NULL;
 }
 
 memcached_return memcached_mget(memcached_st *ptr, 
@@ -382,16 +381,16 @@ memcached_result_st *memcached_fetch_result(memcached_st *ptr,
     }
     else if (*error == MEMCACHED_END && memcached_string_length((memcached_string_st *)(&result->value)) == 0)
     {
-      goto error;
+      break;
     }
     else if (*error == MEMCACHED_END)
     {
       WATCHPOINT_ASSERT(0); /* If this happens we have somehow messed up the fetch */
-      goto error;
+      break;
     }
     else if (*error != MEMCACHED_SUCCESS)
     {
-      goto error;
+      break;
     }
     else
     {
@@ -399,8 +398,11 @@ memcached_result_st *memcached_fetch_result(memcached_st *ptr,
     }
   }
 
-error:
-  memcached_string_reset(&result->value);
+  /* An error has occurred */
+  if (result->is_allocated == MEMCACHED_ALLOCATED)
+    memcached_result_free(result);
+  else
+    memcached_string_reset(&result->value);
 
   return NULL;
 }

diff --git a/lib/memcached_hosts.c b/lib/memcached_hosts.c
index bd9e29c755ba17124789a8cc40a5016a3530e9a1..fac037070c8a6e2b7da59a3efe74a8668eb93d02 100644 (file)
--- a/lib/memcached_hosts.c
+++ b/lib/memcached_hosts.c
@@ -6,11 +6,11 @@ static memcached_return server_add(memcached_st *ptr, char *hostname,
                                    unsigned int port,
                                    memcached_connection type);
 
-static void host_reset(memcached_server_st *host, char *new_hostname, unsigned int port,
+static void host_reset(memcached_server_st *host, char *hostname, unsigned int port,
                        memcached_connection type)
 {
   memset(host,  0, sizeof(memcached_server_st));
-  host->hostname= new_hostname;
+  host->hostname= strdup(hostname);
   host->port= port;
   host->fd= -1;
   host->type= type;
@@ -22,35 +22,31 @@ static void host_reset(memcached_server_st *host, char *new_hostname, unsigned i
 memcached_return memcached_server_push(memcached_st *ptr, memcached_server_st *list)
 {
   unsigned int x;
-  unsigned int count;
+  uint16_t count;
   memcached_server_st *new_host_list;
 
   if (!list)
     return MEMCACHED_SUCCESS;
 
-  for (count= 0; list[count].hostname; count++);
+  count= list[0].count;
 
   new_host_list= 
     (memcached_server_st *)realloc(ptr->hosts, 
-                                   sizeof(memcached_server_st) * (count + ptr->number_of_hosts + 1));
+                                   sizeof(memcached_server_st) * (count + ptr->number_of_hosts));
 
   if (!new_host_list)
     return MEMCACHED_MEMORY_ALLOCATION_FAILURE;
 
   ptr->hosts= new_host_list;
                                    
-  for (x= 0; list[x].hostname; x++)
+  for (x= 0; x < count; x++)
   {
-    ptr->hosts[ptr->number_of_hosts].hostname= strdup(list[x].hostname);
-    ptr->hosts[ptr->number_of_hosts].port= list[x].port;
-    ptr->hosts[ptr->number_of_hosts].fd= list[x].fd;
-    ptr->hosts[ptr->number_of_hosts].stack_responses= list[x].stack_responses;
-    ptr->hosts[ptr->number_of_hosts].cursor_active= list[x].cursor_active;
-    ptr->hosts[ptr->number_of_hosts].type= list[x].type;
+    WATCHPOINT_ASSERT(list[x].hostname);
+    host_reset(&ptr->hosts[ptr->number_of_hosts], list[x].hostname, 
+               list[x].port, list[x].type);
     ptr->number_of_hosts++;
   }
-  host_reset(&ptr->hosts[ptr->number_of_hosts], NULL, 0,
-             MEMCACHED_CONNECTION_UNKNOWN);
+  ptr->hosts[0].count= ptr->number_of_hosts;
 
   return MEMCACHED_SUCCESS;
 }
@@ -94,41 +90,19 @@ static memcached_return server_add(memcached_st *ptr, char *hostname,
                                    memcached_connection type)
 {
   memcached_server_st *new_host_list;
-  char *new_hostname;
   LIBMEMCACHED_MEMCACHED_SERVER_ADD_START();
 
 
-  if (ptr->number_of_hosts)
-  {
-    new_host_list= (memcached_server_st *)realloc(ptr->hosts, 
-                                                  sizeof(memcached_server_st) * (ptr->number_of_hosts+1));
-    if (!new_host_list)
-      return MEMCACHED_MEMORY_ALLOCATION_FAILURE;
-    host_reset(&new_host_list[ptr->number_of_hosts], NULL, 0, 
-               MEMCACHED_CONNECTION_UNKNOWN);
-  }
-  else
-  {
-    new_host_list= 
-      (memcached_server_st *)malloc(sizeof(memcached_server_st) * 2);
-    if (!new_host_list)
-      return MEMCACHED_MEMORY_ALLOCATION_FAILURE;
-    host_reset(&new_host_list[0], NULL, 0, MEMCACHED_CONNECTION_UNKNOWN);
-    host_reset(&new_host_list[1], NULL, 0, MEMCACHED_CONNECTION_UNKNOWN);
-  }
+  new_host_list= (memcached_server_st *)realloc(ptr->hosts, 
+                                                sizeof(memcached_server_st) * (ptr->number_of_hosts+1));
+  if (!new_host_list)
+    return MEMCACHED_MEMORY_ALLOCATION_FAILURE;
 
   ptr->hosts= new_host_list;
 
-  new_hostname=
-    (char *)malloc(sizeof(char) * (strlen(hostname)+1));
-
-  if (!new_hostname)
-    return MEMCACHED_MEMORY_ALLOCATION_FAILURE;
-
-  memcpy(new_hostname, hostname, strlen(hostname));
-  new_hostname[strlen(hostname)]= 0;
-  host_reset(&ptr->hosts[ptr->number_of_hosts], new_hostname, port, type);
+  host_reset(&ptr->hosts[ptr->number_of_hosts], hostname, port, type);
   ptr->number_of_hosts++;
+  ptr->hosts[0].count++;
 
   LIBMEMCACHED_MEMCACHED_SERVER_ADD_END();
 
@@ -141,7 +115,6 @@ memcached_server_st *memcached_server_list_append(memcached_server_st *ptr,
 {
   unsigned int count;
   memcached_server_st *new_host_list;
-  char *new_hostname;
 
   if (!hostname)
     return ptr;
@@ -149,55 +122,45 @@ memcached_server_st *memcached_server_list_append(memcached_server_st *ptr,
   if (!port)
     port= MEMCACHED_DEFAULT_PORT; 
 
-  /* Always count so that we keep a free host at the end */
-  if (ptr)
+  /* Increment count for hosts */
+  count= 1;
+  if (ptr != NULL)
   {
-    for (count= 0; ptr[count].hostname; count++);
-    count+= 2;
-    new_host_list= (memcached_server_st *)realloc(ptr, sizeof(memcached_server_st) * count);
-    if (!new_host_list)
-      goto error;
-    host_reset(&new_host_list[count-1], NULL, 0, MEMCACHED_CONNECTION_UNKNOWN);
-  }
-  else
+    count+= ptr[0].count;
+  } 
+
+  new_host_list= (memcached_server_st *)realloc(ptr, sizeof(memcached_server_st) * count);
+  if (!new_host_list)
   {
-    count= 2;
-    new_host_list= (memcached_server_st *)malloc(sizeof(memcached_server_st) * count);
-    if (!new_host_list)
-      goto error;
-    host_reset(&new_host_list[0], NULL, 0, MEMCACHED_CONNECTION_UNKNOWN);
-    host_reset(&new_host_list[1], NULL, 0, MEMCACHED_CONNECTION_UNKNOWN);
+    *error= MEMCACHED_MEMORY_ALLOCATION_FAILURE;
+    return NULL;
   }
 
-  new_hostname= strdup(hostname);
-
-  if (!new_hostname)
-    goto error;
+  host_reset(&new_host_list[count-1], hostname, port, MEMCACHED_CONNECTION_TCP);
+  new_host_list[0].count++;
 
-  host_reset(&new_host_list[count-2], new_hostname, port, MEMCACHED_CONNECTION_TCP);
 
   *error= MEMCACHED_SUCCESS;
   return new_host_list;
-error:
-  *error= MEMCACHED_MEMORY_ALLOCATION_FAILURE;
-
-  return NULL;
 }
 
 unsigned int memcached_server_list_count(memcached_server_st *ptr)
 {
-  unsigned int x;
 
-  for (x= 0; ptr[x].hostname; x++);
-  
-  return x;
+  if (ptr == NULL)
+    return 0;
+
+  return ptr[0].count;
 }
 
 void memcached_server_list_free(memcached_server_st *ptr)
 {
   unsigned int x;
 
-  for (x= 0; ptr[x].hostname; x++)
+  if (ptr == NULL)
+    return;
+
+  for (x= 0; x < ptr[0].count; x++)
     free(ptr[x].hostname);
 
   free(ptr);

diff --git a/lib/memcached_response.c b/lib/memcached_response.c
index 0755ae0c191cdeb61f217f330433c3ac2c8728ab..78b314f42dc47864c15cc622ceea7b2a9f25cdba 100644 (file)
--- a/lib/memcached_response.c
+++ b/lib/memcached_response.c
@@ -44,6 +44,8 @@ memcached_return memcached_response(memcached_st *ptr,
       total_length++;
       WATCHPOINT_ASSERT(total_length < buffer_length);
     }
+    buffer_ptr++;
+    *buffer_ptr= 0;
 
     if (memcached_server_response_count(ptr, server_key))
       memcached_server_response_decrement(ptr, server_key);

diff --git a/tests/function.c b/tests/function.c
index 3e3b4af4889c7c98e4c4f0d7f73a276a9445a30d..3819eae71e598458d155db98824965d584fb9f0b 100644 (file)
--- a/tests/function.c
+++ b/tests/function.c
@@ -148,6 +148,7 @@ uint8_t append_test(memcached_st *memc)
   assert(!memcmp(value, "we the people", strlen("we the people")));
   assert(strlen("we the people") == value_length);
   assert(rc == MEMCACHED_SUCCESS);
+  free(value);
 
   return 0;
 }
@@ -188,11 +189,12 @@ uint8_t append_binary_test(memcached_st *memc)
 
   store_ptr= (unsigned int *)value;
   x= 0;
-  while (*store_ptr)
+  while ((size_t)store_ptr < (size_t)(value + value_length))
   {
     assert(*store_ptr == store_list[x++]);
     store_ptr++;
   }
+  free(value);
 
   return 0;
 }
@@ -311,6 +313,7 @@ uint8_t prepend_test(memcached_st *memc)
   assert(!memcmp(value, "we the people", strlen("we the people")));
   assert(strlen("we the people") == value_length);
   assert(rc == MEMCACHED_SUCCESS);
+  free(value);
 
   return 0;
 }
@@ -1412,6 +1415,13 @@ uint8_t string_alloc_append_toobig(memcached_st *memc)
   return 0;
 }
 
+uint8_t cleanup_pairs(memcached_st *memc)
+{
+  pairs_free(global_pairs);
+
+  return 0;
+}
+
 uint8_t generate_data(memcached_st *memc)
 {
   unsigned long long x;
@@ -1765,6 +1775,7 @@ test_st generate_tests[] ={
   {"get_read", 0, get_read },
   {"mget_read", 0, mget_read },
   {"mget_read_result", 0, mget_read_result },
+  {"cleanup", 0, cleanup_pairs },
   {0, 0, 0}
 };