I cherry-picked Brad's work into a branch and got it passing all the
tests on all the platforms for which I have builders. I had to write
some new ones along the way, but I think it's good.
What it basically does is ensure that
a) incr and decr operations have valid incr and decr amounts
(e.g. you can't pass "a lot" anymore).
b) incr and decr are operating on numeric data.
Before, you could store a picture of a zero in a key and increment
it. More generally, memcached didn't care what was stored in a value
for incr/decr. That's silly behavior and if your app relied on it, it
probably has all kinds of other bugs.
This is a semantic change, though, so before it goes in, I'd like
people to take a look a it.
It's in the "incr_fix" branch of my repo on github:
http://github.com/dustin/memcached/tree/incr_fix
A quick diff (ignoring the changesets along the way) can be found
here:
http://gist.github.com/79353
or you can read it inline here:
diff --git a/.gitignore b/.gitignore
index affe663..195b246 100644
--- a/.gitignore
+++ b/.gitignore
@@ -33,3 +33,4 @@ memcached-*.tar.gz
doc/protocol-binary-range.txt
doc/protocol-binary.txt
/sizes
+/internal_tests
\ No newline at end of file
diff --git a/Makefile.am b/Makefile.am
index ab4c26c..d263d07 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,4 +1,4 @@
-bin_PROGRAMS = memcached memcached-debug sizes
+bin_PROGRAMS = memcached memcached-debug sizes internal_tests
pkginclude_HEADERS = protocol_binary.h
BUILT_SOURCES=
@@ -10,6 +10,7 @@ memcached_SOURCES = memcached.c memcached.h \
assoc.c assoc.h \
thread.c daemon.c \
stats.c stats.h \
+ util.c util.h \
trace.h
if BUILD_SOLARIS_PRIVS
@@ -26,6 +27,8 @@ memcached_DEPENDENCIES =
memcached_debug_DEPENDENCIES =
CLEANFILES=
+internal_tests_SOURCES = internal_tests.c util.c
+
if BUILD_DTRACE
BUILT_SOURCES += memcached_dtrace.h
CLEANFILES += memcached_dtrace.h
@@ -58,8 +61,9 @@ EXTRA_DIST = doc scripts TODO t memcached.spec
memcached_dtrace.d
MOSTLYCLEANFILES = *.gcov *.gcno *.gcda *.tcov
-test: memcached-debug sizes
+test: memcached-debug internal_tests sizes
$(srcdir)/sizes
+ $(srcdir)/internal_tests
prove $(srcdir)/t
@if test `basename $(PROFILER)` = "gcov"; then \
for file in memcached_debug-*.gc??; do \
diff --git a/doc/protocol.txt b/doc/protocol.txt
index a39a7f3..9bfe901 100644
--- a/doc/protocol.txt
+++ b/doc/protocol.txt
@@ -275,11 +275,12 @@ Increment/Decrement
Commands "incr" and "decr" are used to change data for some item
in-place, incrementing or decrementing it. The data for the item is
-treated as decimal representation of a 64-bit unsigned integer. If
the
-current data value does not conform to such a representation, the
-commands behave as if the value were 0. Also, the item must already
-exist for incr/decr to work; these commands won't pretend that a
-non-existent key exists with value 0; instead, they will fail.
+treated as decimal representation of a 64-bit unsigned integer. If
+the current data value does not conform to such a representation, the
+incr/decr commands return an error (memcached <= 1.2.6 treated the
+bogus value as if it were 0, leading to confusing). Also, the item
+must already exist for incr/decr to work; these commands won't
pretend
+that a non-existent key exists with value 0; instead, they will fail.
The client sends the command line:
diff --git a/globals.c b/globals.c
new file mode 100644
index 0000000..7d7b2a3
--- /dev/null
+++ b/globals.c
@@ -0,0 +1,23 @@
+#include "memcached.h"
+
+/*
+ * This file contains global variables shared across the rest of the
+ * memcached codebase. These were originally in memcached.c but had
+ * to be removed to make the rest of the object files linkable into
+ * the test infrastructure.
+ *
+ */
+
+/*
+ * We keep the current time of day in a global variable that's
updated by a
+ * timer event. This saves us a bunch of time() system calls (we
really only
+ * need to get the time once a second, whereas there can be tens of
thousands
+ * of requests a second) and allows us to use server-start-relative
timestamps
+ * rather than absolute UNIX timestamps, a space savings on systems
where
+ * sizeof(time_t) > sizeof(unsigned int).
+ */
+volatile rel_time_t current_time;
+
+/** exported globals **/
+struct stats stats;
+struct settings settings;
diff --git a/internal_tests.c b/internal_tests.c
new file mode 100644
index 0000000..0fcddef
--- /dev/null
+++ b/internal_tests.c
@@ -0,0 +1,62 @@
+/* -*- Mode: C; tab-width: 4; c-basic-offset: 4; indent-tabs-mode:
nil -*- */
+
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+#include "memcached.h"
+
+static void test_safe_strtoull(void);
+static void test_safe_strtoll(void);
+
+static void test_safe_strtoull() {
+ uint64_t val;
+ assert(safe_strtoull("123", &val));
+ assert(val == 123);
+ assert(safe_strtoull("+123", &val));
+ assert(val == 123);
+ assert(!safe_strtoull("", &val)); // empty
+ assert(!safe_strtoull("123BOGUS", &val)); // non-numeric
+ assert(!safe_strtoull("92837498237498237498029383", &val)); // out
of range
+
+ // extremes:
+ assert(safe_strtoull("18446744073709551615", &val)); // 2**64 - 1
+ assert(val == 18446744073709551615ULL);
+ assert(!safe_strtoull("18446744073709551616", &val)); // 2**64
+ assert(!safe_strtoull("-1", &val)); // negative
+}
+
+static void test_safe_strtoll() {
+ int64_t val;
+ assert(safe_strtoll("123", &val));
+ assert(val == 123);
+ assert(safe_strtoll("+123", &val));
+ assert(val == 123);
+ assert(safe_strtoll("-123", &val));
+ assert(val == -123);
+ assert(!safe_strtoll("", &val)); // empty
+ assert(!safe_strtoll("123BOGUS", &val)); // non-numeric
+ assert(!safe_strtoll("92837498237498237498029383", &val)); // out
of range
+
+ // extremes:
+ assert(!safe_strtoll("18446744073709551615", &val)); // 2**64 - 1
+ assert(safe_strtoll("9223372036854775807", &val)); // 2**63 - 1
+ assert(val == 9223372036854775807LL);
+ /*
+ assert(safe_strtoll("-9223372036854775808", &val)); // -2**63
+ assert(val == -9223372036854775808LL);
+ */
+ assert(!safe_strtoll("-9223372036854775809", &val)); // -2**63 - 1
+
+ // We'll allow space to terminate the string. And leading space.
+ assert(safe_strtoll(" 123 foo", &val));
+ assert(val == 123);
+
+}
+
+int main(int argc, char **argv) {
+ test_safe_strtoull();
+ test_safe_strtoll();
+ printf("OK.\n");
+ return 0;
+}
diff --git a/memcached.c b/memcached.c
index be9f2c2..b7ec0a2 100644
--- a/memcached.c
+++ b/memcached.c
@@ -2621,12 +2621,12 @@ static void process_update_command(conn *c,
token_t *tokens, const size_t ntoken
vlen = strtol(tokens[4].value, NULL, 10);
// does cas value exist?
- if(handle_cas) {
- req_cas_id = strtoull(tokens[5].value, NULL, 10);
+ if (handle_cas) {
+ req_cas_id = strtoull(tokens[5].value, NULL, 10);
}
- if(errno == ERANGE || ((flags == 0 || exptime == 0) && errno ==
EINVAL)
- || vlen < 0) {
+ if (errno == ERANGE || ((flags == 0 || exptime == 0) && errno ==
EINVAL)
+ || vlen < 0) {
out_string(c, "CLIENT_ERROR bad command line format");
return;
}
@@ -2670,7 +2670,7 @@ static void process_update_command(conn *c,
token_t *tokens, const size_t ntoken
static void process_arithmetic_command(conn *c, token_t *tokens,
const size_t ntokens, const bool incr) {
char temp[sizeof("18446744073709551615")];
item *it;
- int64_t delta;
+ uint64_t delta;
char *key;
size_t nkey;
@@ -2678,7 +2678,7 @@ static void process_arithmetic_command(conn *c,
token_t *tokens, const size_t nt
set_noreply_maybe(c, tokens, ntokens);
- if(tokens[KEY_TOKEN].length > KEY_MAX_LENGTH) {
+ if (tokens[KEY_TOKEN].length > KEY_MAX_LENGTH) {
out_string(c, "CLIENT_ERROR bad command line format");
return;
}
@@ -2686,10 +2686,8 @@ static void process_arithmetic_command(conn *c,
token_t *tokens, const size_t nt
key = tokens[KEY_TOKEN].value;
nkey = tokens[KEY_TOKEN].length;
- delta = strtoll(tokens[2].value, NULL, 10);
-
- if(errno == ERANGE) {
- out_string(c, "CLIENT_ERROR bad command line format");
+ if (!safe_strtoull(tokens[2].value, &delta)) {
+ out_string(c, "CLIENT_ERROR invalid numeric delta argument");
return;
}
@@ -2728,11 +2726,8 @@ char *do_add_delta(conn *c, item *it, const
bool incr, const int64_t delta, char
int res;
ptr = ITEM_data(it);
- while ((*ptr != '\0') && (*ptr < '0' && *ptr > '9')) ptr++; //
BUG: can't be true
-
- value = strtoull(ptr, NULL, 10);
- if(errno == ERANGE) {
+ if (!safe_strtoull(ptr, &value)) {
return "CLIENT_ERROR cannot increment or decrement non-
numeric value";
}
diff --git a/memcached.h b/memcached.h
index 2378848..f3d0a19 100644
--- a/memcached.h
+++ b/memcached.h
@@ -24,7 +24,7 @@
#define UDP_MAX_PAYLOAD_SIZE 1400
#define UDP_HEADER_SIZE 8
#define MAX_SENDBUF_SIZE (256 * 1024 * 1024)
-/* I'm told the max legnth of a 64-bit num converted to string is 20
bytes.
+/* I'm told the max length of a 64-bit num converted to string is 20
bytes.
* Plus a few for spaces, \r\n, \0 */
#define SUFFIX_SIZE 24
@@ -336,7 +336,7 @@ extern int daemonize(int nochdir, int noclose);
#include "items.h"
#include "trace.h"
#include "hash.h"
-
+#include "util.h"
/*
* Functions such as the libevent-related calls that need to do cross-
thread
diff --git a/t/incrdecr.t b/t/incrdecr.t
index bce9af3..e0ba65f 100755
--- a/t/incrdecr.t
+++ b/t/incrdecr.t
@@ -1,7 +1,7 @@
#!/usr/bin/perl
use strict;
-use Test::More tests => 21;
+use Test::More tests => 23;
use FindBin qw($Bin);
use lib "$Bin/lib";
use MemcachedTest;
@@ -58,8 +58,14 @@ is(scalar <$sock>, "NOT_FOUND\r\n", "can't decr
bogus key");
print $sock "decr incr 5\r\n";
is(scalar <$sock>, "NOT_FOUND\r\n", "can't incr bogus key");
+print $sock "set bigincr 0 0 1\r\n0\r\n";
+is(scalar <$sock>, "STORED\r\n", "stored bigincr");
+print $sock "incr bigincr 18446744073709551610\r\n";
+is(scalar <$sock>, "18446744073709551610\r\n");
+
print $sock "set text 0 0 2\r\nhi\r\n";
-is(scalar <$sock>, "STORED\r\n", "stored text");
+is(scalar <$sock>, "STORED\r\n", "stored hi");
print $sock "incr text 1\r\n";
-is(scalar <$sock>, "1\r\n", "hi - 1 = 0");
-
+is(scalar <$sock>,
+ "CLIENT_ERROR cannot increment or decrement non-numeric value\r
\n",
+ "hi - 1 = 0");
diff --git a/util.c b/util.c
new file mode 100644
index 0000000..a2d0728
--- /dev/null
+++ b/util.c
@@ -0,0 +1,46 @@
+#include <stdlib.h>
+#include <assert.h>
+#include <ctype.h>
+#include <errno.h>
+#include <string.h>
+#include <stdlib.h>
+
+#include "memcached.h"
+
+bool safe_strtoull(const char *str, uint64_t *out) {
+ assert(out != NULL);
+ errno = 0;
+ *out = 0;
+ char *endptr;
+ unsigned long long ull = strtoull(str, &endptr, 10);
+ if (errno == ERANGE)
+ return false;
+ if (isspace(*endptr) || (*endptr == '\0' && endptr != str)) {
+ if ((long long) ull < 0) {
+ /* only check for negative signs in the uncommon case
when
+ * the unsigned number is so big that it's negative as a
+ * signed number. */
+ if (strchr(str, '-') != NULL) {
+ return false;
+ }
+ }
+ *out = ull;
+ return true;
+ }
+ return false;
+}
+
+bool safe_strtoll(const char *str, int64_t *out) {
+ assert(out != NULL);
+ errno = 0;
+ *out = 0;
+ char *endptr;
+ long long ll = strtoll(str, &endptr, 10);
+ if (errno == ERANGE)
+ return false;
+ if (isspace(*endptr) || (*endptr == '\0' && endptr != str)) {
+ *out = ll;
+ return true;
+ }
+ return false;
+}
diff --git a/util.h b/util.h
new file mode 100644
index 0000000..b5a043f
--- /dev/null
+++ b/util.h
@@ -0,0 +1,11 @@
+/*
+ * Wrappers around strtoull/strtoll that are safer and easier to
+ * use. For tests and assumptions, see internal_tests.c.
+ *
+ * str a NULL-terminated base decimal 10 unsigned integer
+ * out out parameter, if conversion succeeded
+ *
+ * returns true if conversion succeeded.
+ */
+bool safe_strtoull(const char *str, uint64_t *out);
+bool safe_strtoll(const char *str, int64_t *out);
