Twemcache 2.5.3 release

ChangeLog

@@ -1,3 +1,13 @@
+2012-12-11  Cache Team <[email protected]>
+	* twemcache: version 2.5.3 release
+	* Fix: insufficient item length causing segfault
+	* Fix: get/delete race causing segfault
+	* Fix: inconsistency in prepend/append replies under concurrency
+	* Fix: update cas for in-place prepend/append
+	* Fix: strtoull function taking length as a parameter to avoid overrun
+	* Debug: memory-scrubbing enabled in full debug build
+	* Misc: soarpenguin's several from https://github.com/twitter/twemcache/pull/15
+
 2012-09-24  Cache Team <[email protected]>
 	* twemcache: version 2.5.2 release
 	* Feature: command line option for klog sampling rate, default is now 100

configure.ac

@@ -1,7 +1,7 @@
 # Define the package version numbers and the bug reporting address
 m4_define([MC_MAJOR], 2)
 m4_define([MC_MINOR], 5)
-m4_define([MC_PATCH], 2)
+m4_define([MC_PATCH], 3)
 m4_define([MC_BUGS], [[email protected]])
 
 # Initialize autoconf
@@ -101,6 +101,7 @@ AS_CASE([x$enable_debug],
   [xfull], [AC_DEFINE([HAVE_ASSERT_PANIC], [1],
                       [Define to 1 if panic on an assert is enabled])
             AC_DEFINE([HAVE_DEBUG_LOG], [1], [Define to 1 if debug log is enabled])
+            AC_DEFINE([HAVE_MEM_SCRUB], [1], [Define to 1 if memory scrubbing is enabled])
            ],
   [xyes], [AC_DEFINE([HAVE_ASSERT_LOG], [1],
                      [Define to 1 if log on an assert is enabled])

src/mc_ascii.c

@@ -100,7 +100,7 @@ extern struct settings settings;
 #define TOKEN_KLOG_SUBCOMMAND   3
 #define TOKEN_MAX               8
 
-#define SUFFIX_MAX_LEN 32 /* enough to hold "<uint32_t> <uint32_t> \r\n" */
+#define SUFFIX_MAX_LEN 44 /* =11+11+21+1 enough to hold " <uint32_t> <uint32_t> <uint64_t>\0" */
 
 struct token {
     char   *val; /* token value */
@@ -429,7 +429,7 @@ asc_complete_nread(struct conn *c)
       }
     }
 
-    item_remove(c->item);
+    item_remove(it);
     c->item = NULL;
 }
 
@@ -492,11 +492,14 @@ asc_respond_get(struct conn *c, unsigned valid_key_iter, struct item *it,
     char *suffix = NULL;
     int sz;
     int total_len = 0;
+    uint32_t nbyte = it->nbyte;
+    char *data = item_data(it);
 
-    status = conn_add_iov(c, "VALUE ", sizeof("VALUE ") - 1);
+    status = conn_add_iov(c, VALUE, VALUE_LEN);
     if (status != MC_OK) {
         return status;
     }
+    total_len += VALUE_LEN;
 
     status = conn_add_iov(c, item_key(it), it->nkey);
     if (status != MC_OK) {
@@ -510,12 +513,12 @@ asc_respond_get(struct conn *c, unsigned valid_key_iter, struct item *it,
     }
     if (return_cas) {
         sz = mc_snprintf(suffix, SUFFIX_MAX_LEN, " %"PRIu32" %"PRIu32" %"PRIu64,
-                      it->dataflags, it->nbyte, item_cas(it));
-        ASSERT(sz < SUFFIX_SIZE + CAS_SUFFIX_SIZE);
+                      it->dataflags, nbyte, item_cas(it));
+        ASSERT(sz <= SUFFIX_SIZE + CAS_SUFFIX_SIZE);
      } else {
         sz = mc_snprintf(suffix, SUFFIX_MAX_LEN, " %"PRIu32" %"PRIu32,
-                      it->dataflags, it->nbyte);
-        ASSERT(sz < SUFFIX_SIZE);
+                      it->dataflags, nbyte);
+        ASSERT(sz <= SUFFIX_SIZE);
     }
     if (sz < 0) {
         return MC_ERROR;
@@ -533,11 +536,11 @@ asc_respond_get(struct conn *c, unsigned valid_key_iter, struct item *it,
     }
     total_len += CRLF_LEN;
 
-    status = conn_add_iov(c, item_data(it), it->nbyte);
+    status = conn_add_iov(c, data, nbyte);
     if (status != MC_OK) {
         return status;
     }
-    total_len += it->nbyte;
+    total_len += nbyte;
 
     status = conn_add_iov(c, CRLF, CRLF_LEN);
     if (status != MC_OK) {
@@ -686,7 +689,8 @@ static void
 asc_process_update(struct conn *c, struct token *token, int ntoken)
 {
     char *key;
-    size_t nkey;
+    size_t keylen;
+    uint8_t nkey;
     uint32_t flags, vlen;
     int32_t exptime_int;
     time_t exptime;
@@ -710,14 +714,16 @@ asc_process_update(struct conn *c, struct token *token, int ntoken)
     type = c->req_type;
     handle_cas = (type == REQ_CAS) ? true : false;
     key = token[TOKEN_KEY].val;
-    nkey = token[TOKEN_KEY].len;
+    keylen = token[TOKEN_KEY].len;
 
-    if (nkey > KEY_MAX_LEN) {
+    if (keylen > KEY_MAX_LEN) {
         log_debug(LOG_NOTICE, "client error on c %d for req of type %d and %d "
-                  "length key", c->sd, c->req_type, nkey);
+                  "length key", c->sd, c->req_type, keylen);
 
         asc_write_client_error(c);
         return;
+    } else {
+        nkey = (uint8_t)keylen;
     }
 
     if (!mc_strtoul(token[TOKEN_FLAGS].val, &flags)) {
@@ -913,6 +919,10 @@ asc_process_delete(struct conn *c, struct token *token, int ntoken)
         return;
     }
 
+    /*
+     * FIXME: This is not thread-safe, two threads could try to delete the same
+     * item twice after succeeding in item_get, leading to erroneous stats
+     */
     it = item_get(key, nkey);
     if (it != NULL) {
         stats_slab_incr(it->id, delete_hit);

src/mc_core.h

@@ -51,6 +51,12 @@
 # define MC_ASSERT_LOG 0
 #endif
 
+#ifdef HAVE_MEM_SCRUB
+# define MC_MEM_SCRUB 1
+#else
+# define MC_MEM_SCRUB 0
+#endif
+
 #ifdef DISABLE_STATS
 # define MC_DISABLE_STATS 1
 #else
@@ -178,7 +184,7 @@ struct slabclass;
  * suffix is also given an extra byte to store '\0'
  */
 #define CAS_SUFFIX_SIZE 21 /* of the cas field only */
-#define SUFFIX_SIZE     23 /* of the other fields */
+#define SUFFIX_SIZE     22 /* of the other fields */
 
 /* size of an incr buf */
 #define INCR_MAX_STORAGE_LEN 24

src/mc_items.c

@@ -356,12 +356,12 @@ _item_alloc(uint8_t id, char *key, uint8_t nkey, uint32_t dataflags, rel_time_t
 
     it = slab_get_item(id);
     if (it != NULL) {
-        /* 2) or 3a) either we allow random eviction a free item is found */
+        /* 2) or 3) either we allow random eviction a free item is found */
         goto done;
     }
 
     if (uit != NULL) {
-        /* 3b) this is an lru item and we can reuse it */
+        /* 4) this is an lru item and we can reuse it */
         it = uit;
         stats_slab_incr(id, item_evict);
         stats_slab_settime(id, item_evict_ts, time_now());
@@ -391,6 +391,9 @@ _item_alloc(uint8_t id, char *key, uint8_t nkey, uint32_t dataflags, rel_time_t
     it->nbyte = nbyte;
     it->exptime = exptime;
     it->nkey = nkey;
+#if defined MC_MEM_SCRUB && MC_MEM_SCRUB == 1
+    memset(it->end, 0xff, slab_item_size(it->id) - ITEM_HDR_SIZE);
+#endif
     memcpy(item_key(it), key, nkey);
 
     stats_slab_incr(id, item_acquire);
@@ -403,13 +406,13 @@ _item_alloc(uint8_t id, char *key, uint8_t nkey, uint32_t dataflags, rel_time_t
 }
 
 struct item *
-item_alloc(uint8_t id, char *key, size_t nkey, uint32_t flags,
+item_alloc(uint8_t id, char *key, uint8_t nkey, uint32_t dataflags,
            rel_time_t exptime, uint32_t nbyte)
 {
     struct item *it;
 
     pthread_mutex_lock(&cache_lock);
-    it = _item_alloc(id, key, nkey, flags, exptime, nbyte);
+    it = _item_alloc(id, key, nkey, dataflags, exptime, nbyte);
     pthread_mutex_unlock(&cache_lock);
 
     return it;
@@ -531,10 +534,10 @@ _item_touch(struct item *it)
               "%02x id %"PRId8"", it->nkey, item_key(it), it->offset,
               it->flags, it->id);
 
-    ASSERT(item_is_linked(it));
-
-    item_unlink_q(it);
-    item_link_q(it, false);
+    if (item_is_linked(it)) {
+        item_unlink_q(it);
+        item_link_q(it, false);
+    }
 }
 
 void
@@ -823,6 +826,10 @@ _item_store(struct item *it, req_type_t type, struct conn *c)
                 log_debug(LOG_NOTICE, "client error on c %d for req of type %d"
                           " with key size %"PRIu8" and value size %"PRIu32,
                           c->sd, c->req_type, oit->nkey, total_nbyte);
+
+                stats_thread_incr(cmd_error);
+                store_it = false;
+                break;
             }
 
             /* if oit is large enough to hold the extra data and left-aligned,
@@ -834,6 +841,8 @@ _item_store(struct item *it, req_type_t type, struct conn *c)
                 memcpy(item_data(oit) + oit->nbyte, item_data(it), it->nbyte);
                 oit->nbyte = total_nbyte;
                 it = oit;
+                item_set_cas(it, item_next_cas());
+                store_it = false;
             } else {
                 nit = _item_alloc(id, key, oit->nkey, oit->dataflags,
                                   oit->exptime, total_nbyte);
@@ -862,6 +871,10 @@ _item_store(struct item *it, req_type_t type, struct conn *c)
                 log_debug(LOG_NOTICE, "client error on c %d for req of type %d"
                           " with key size %"PRIu8" and value size %"PRIu32,
                           c->sd, c->req_type, oit->nkey, total_nbyte);
+
+                stats_thread_incr(cmd_error);
+                store_it = false;
+                break;
             }
             /* if oit is large enough to hold the extra data and is already
              * right-aligned, we copy the delta to the front of the existing
@@ -872,6 +885,8 @@ _item_store(struct item *it, req_type_t type, struct conn *c)
                 memcpy(item_data(oit) - it->nbyte, item_data(it), it->nbyte);
                 oit->nbyte = total_nbyte;
                 it = oit;
+                item_set_cas(it, item_next_cas());
+                store_it = false;
             } else {
                 nit = _item_alloc(id, key, oit->nkey, oit->dataflags,
                                   oit->exptime, total_nbyte);
@@ -915,6 +930,10 @@ _item_store(struct item *it, req_type_t type, struct conn *c)
             _item_link(it);
         }
         result = STORED;
+
+        log_debug(LOG_VERB, "store it '%.*s'at offset %"PRIu32" with flags %02x"
+                  " id %"PRId8"", it->nkey, item_key(it), it->offset, it->flags,
+                  it->id);
     }
 
     /* release our reference, if any */
@@ -960,7 +979,7 @@ _item_add_delta(struct conn *c, char *key, size_t nkey, bool incr,
 
     ptr = item_data(it);
 
-    if (!mc_strtoull(ptr, &value)) {
+    if (!mc_strtoull_len(ptr, &value, it->nbyte)) {
         _item_remove(it);
         return DELTA_NON_NUMERIC;
     }
@@ -1019,9 +1038,8 @@ _item_add_delta(struct conn *c, char *key, size_t nkey, bool incr,
         }
 
         item_set_cas(it, item_next_cas());
-
         memcpy(item_data(it), buf, res);
-        memset(item_data(it) + res, ' ', it->nbyte - res);
+        it->nbyte = res;
     }
 
     _item_remove(it);

src/mc_items.h

@@ -199,7 +199,7 @@ item_ntotal(uint8_t nkey, uint32_t nbyte, bool use_cas)
     size_t ntotal;
 
     ntotal = use_cas ? sizeof(uint64_t) : 0;
-    ntotal += ITEM_HDR_SIZE + nkey + 1 + nbyte;
+    ntotal += ITEM_HDR_SIZE + nkey + 1 + nbyte + CRLF_LEN;
 
     return ntotal;
 }
@@ -222,7 +222,7 @@ struct slab *item_2_slab(struct item *it);
 void item_hdr_init(struct item *it, uint32_t offset, uint8_t id);
 
 uint8_t item_slabid(uint8_t nkey, uint32_t nbyte);
-struct item *item_alloc(uint8_t id, char *key, size_t nkey, uint32_t dataflags, rel_time_t exptime, uint32_t nbyte);
+struct item *item_alloc(uint8_t id, char *key, uint8_t nkey, uint32_t dataflags, rel_time_t exptime, uint32_t nbyte);
 
 void item_reuse(struct item *it);
 

src/mc_thread.c

@@ -256,7 +256,8 @@ thread_setup(struct thread_worker *t)
 
     conn_cq_init(&t->new_cq);
 
-    suffix_size = settings.use_cas ? (CAS_SUFFIX_SIZE + SUFFIX_SIZE) : SUFFIX_SIZE;
+    suffix_size = settings.use_cas ? (CAS_SUFFIX_SIZE + SUFFIX_SIZE + 1) :
+                  (SUFFIX_SIZE + 1);
     t->suffix_cache = cache_create("suffix", suffix_size, sizeof(char *));
     if (t->suffix_cache == NULL) {
         log_error("cache create of suffix cache failed: %s", strerror(errno));

src/mc_util.c

@@ -265,6 +265,44 @@ mc_valid_port(int n)
     return true;
 }
 
+static void
+mc_skip_space(const char **str, size_t *len)
+{
+    while (*len > 0 && isspace(**str)) {
+        (*str)++;
+        (*len)--;
+    }
+}
+
+bool
+mc_strtoull_len(const char *str, uint64_t *out, size_t len)
+{
+    *out = 0ULL;
+
+    mc_skip_space(&str, &len);
+
+    while (len > 0 && (*str) >= '0' && (*str) <= '9') {
+        if (*out >= UINT64_MAX / 10) {
+            /*
+             * At this point the integer is considered out of range,
+             * by doing so we convert integers up to (UINT64_MAX - 6)
+             */
+            return false;
+        }
+        *out = *out * 10 + *str - '0';
+        str++;
+        len--;
+    }
+
+    mc_skip_space(&str, &len);
+
+    if (len == 0) {
+        return true;
+    } else {
+        return false;
+    }
+}
+
 bool
 mc_strtoull(const char *str, uint64_t *out)
 {

src/mc_util.h

@@ -36,6 +36,8 @@
 #define CR              (uint8_t) 13
 #define CRLF            "\x0d\x0a"
 #define CRLF_LEN        (uint32_t) (sizeof(CRLF) - 1)
+#define VALUE           "VALUE "
+#define VALUE_LEN       sizeof(VALUE) - 1
 
 #define NELEMS(a)       ((sizeof(a)) / sizeof((a)[0]))
 
@@ -142,6 +144,7 @@ bool mc_valid_port(int n);
  * Wrappers around strtoull, strtoll, strtoul, strtol that are safer and
  * easier to use. Returns true if conversion succeeds.
  */
+bool mc_strtoull_len(const char *str, uint64_t *out, size_t len);
 bool mc_strtoull(const char *str, uint64_t *out);
 bool mc_strtoll(const char *str, int64_t *out);
 bool mc_strtoul(const char *str, uint32_t *out);

tests/functional/64bit.py

@@ -89,7 +89,7 @@ def test_64bit(self):
         self.assertEqual(0, active)
         for key in range(0, 10):
             size = int(statsettings[0][1]['slab_size']) - ITEM_OVERHEAD - SLAB_OVERHEAD\
-                   - CAS_LEN - len(str(key) + '\0')
+                   - CAS_LEN - len(str(key) + '\0') - 2 # CRLF_LEN
             self.mc.set(str(key), 'a' * size)
             self.assertIsNotNone(self.mc.get(str(key)))
             key += 1

tests/functional/advanced.py

@@ -53,7 +53,7 @@ def tearDown(self):
     def test_itemlru(self):
         ''' test item lru algorithm '''
         args = Args(command='MAX_MEMORY = 8\nEVICTION = 1\nTHREADS = 1') #lru eviction
-        size = SLAB_SIZE - ITEM_OVERHEAD - SLAB_OVERHEAD - CAS_LEN - len("big0\0")
+        size = SLAB_SIZE - ITEM_OVERHEAD - SLAB_OVERHEAD - CAS_LEN - len("big0\0") - 2 #CRLF_LEN needs to be excluded
         data = '0' * size
         self.server = startServer(args)
         self.assertTrue(self.mc.set("big0", data))