This just scrolled off the list page with no commentary. I'm going to assume it's because everybody think it's completely awesome and/or nobody would dare question Brad's wisdom and go ahead and push it in the next day or so.
Keep in mind, this is not a backwards compatible change. Of course, the only thing it "breaks" is code that relies on kind of dumb buggy behavior, so I wouldn't expect anyone to really be worried about it. On Mar 15, 12:32 am, Dustin <[email protected]> wrote: > 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);
