Skip to content
Browse files

Twemcache 2.5.3 release

  • Loading branch information...
1 parent d5d8b76 commit 23b6c0e7647c89d5eb544936660585b4a847b90d @thinkingfish thinkingfish committed
Showing with 136 additions and 31 deletions.
  1. +10 −0 ChangeLog
  2. +2 −1 configure.ac
  3. +23 −13 src/mc_ascii.c
  4. +7 −1 src/mc_core.h
  5. +29 −11 src/mc_items.c
  6. +2 −2 src/mc_items.h
  7. +2 −1 src/mc_thread.c
  8. +38 −0 src/mc_util.c
  9. +3 −0 src/mc_util.h
  10. +1 −1 tests/functional/64bit.py
  11. +1 −1 tests/functional/advanced.py
  12. +7 −0 tests/functional/basic.py
  13. +11 −0 tests/protocol/badbasic.py
View
10 ChangeLog
@@ -1,3 +1,13 @@
+2012-12-11 Cache Team <cache-team@twitter.com>
+ * 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 <cache-team@twitter.com>
* twemcache: version 2.5.2 release
* Feature: command line option for klog sampling rate, default is now 100
View
3 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])
View
36 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);
View
8 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
View
40 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);
View
4 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);
View
3 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));
View
38 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)
{
View
3 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);
View
2 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
View
2 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))
View
7 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'''
View
11 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()

0 comments on commit 23b6c0e

Please sign in to comment.
Something went wrong with that request. Please try again.