incr_fix.diff

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);