diff --git a/ChangeLog b/ChangeLog index fcebd95..7c5e579 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2012-12-11 Cache Team + * 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 * twemcache: version 2.5.2 release * Feature: command line option for klog sampling rate, default is now 100 diff --git a/configure.ac b/configure.ac index cfab40a..b235c16 100644 --- a/configure.ac +++ b/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], [cache-team@twitter.com]) # 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]) diff --git a/src/mc_ascii.c b/src/mc_ascii.c index 63a5577..81b7d00 100644 --- a/src/mc_ascii.c +++ b/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 " \r\n" */ +#define SUFFIX_MAX_LEN 44 /* =11+11+21+1 enough to hold " \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); diff --git a/src/mc_core.h b/src/mc_core.h index c475617..fcace35 100644 --- a/src/mc_core.h +++ b/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 diff --git a/src/mc_items.c b/src/mc_items.c index 39ce399..aad0724 100644 --- a/src/mc_items.c +++ b/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); diff --git a/src/mc_items.h b/src/mc_items.h index 212d7ab..a9a3df1 100644 --- a/src/mc_items.h +++ b/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); diff --git a/src/mc_thread.c b/src/mc_thread.c index db96e7a..626c58e 100644 --- a/src/mc_thread.c +++ b/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)); diff --git a/src/mc_util.c b/src/mc_util.c index bcb064a..169332f 100644 --- a/src/mc_util.c +++ b/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) { diff --git a/src/mc_util.h b/src/mc_util.h index 750018f..20e2153 100644 --- a/src/mc_util.h +++ b/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); diff --git a/tests/functional/64bit.py b/tests/functional/64bit.py index 866e89b..aa8cc9d 100644 --- a/tests/functional/64bit.py +++ b/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 diff --git a/tests/functional/advanced.py b/tests/functional/advanced.py index f5151dd..4cce52f 100644 --- a/tests/functional/advanced.py +++ b/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)) diff --git a/tests/functional/basic.py b/tests/functional/basic.py index abbcffe..afd85b6 100644 --- a/tests/functional/basic.py +++ b/tests/functional/basic.py @@ -241,6 +241,13 @@ def test_incr(self): self.mc.incr("numkey", 2) val = self.mc.get("numkey") self.assertEqual(val, "3") + self.mc.set("numkey", "9") + self.mc.incr("numkey") + val = self.mc.get("numkey") + self.assertEqual(val, "10") + self.mc.incr("numkey") + val = self.mc.get("numkey") + self.assertEqual(val, "11") def test_decr(self): '''numeric: decr''' diff --git a/tests/protocol/badbasic.py b/tests/protocol/badbasic.py index 8e59c05..4b439f3 100644 --- a/tests/protocol/badbasic.py +++ b/tests/protocol/badbasic.py @@ -92,6 +92,17 @@ def test_longkey(self): server.send_cmd("get {0}\r\n".format(key)) # looooooong key self.assertEqual("CLIENT_ERROR", server.expect("CLIENT_ERROR")) + def test_largevalue(self): + '''Append/prepend grows item out of size range.''' + key = 'appendto' + val = '0' * (SLAB_SIZE - SLAB_OVERHEAD - ITEM_OVERHEAD - 100) + self.mc.set(key, val) + self.assertEqual(val, self.mc.get(key)) + delta = '0' * 100 + self.mc.append(key, delta) # delta makes the value out of range, no-op expected + self.assertEqual(val, self.mc.get(key)) + self.mc.prepend(key, delta) # delta makes the value out of range, no-op expected + self.assertEqual(val, self.mc.get(key)) if __name__ == '__main__': server = startServer()