Heh, it's on my to-review list. Lemmie get back to you within the next 24hrs ;)
On Wed, Mar 18, 2009 at 2:39 AM, Dustin <[email protected]> wrote: > > > 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);
