From 30407e1f4fc290468f3d8ec31cb933402767568d Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 15 Dec 2010 00:42:32 +0100 Subject: [PATCH 1/6] Make SETBIT return original bit value --- src/t_string.c | 41 +++++++++++++++++++++++++---------------- tests/unit/basic.tcl | 24 ++++++++++++------------ 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/src/t_string.c b/src/t_string.c index 736b1673d..f1bab4dad 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -111,17 +111,18 @@ void setbitCommand(redisClient *c) { robj *o; char *err = "bit is not an integer or out of range"; size_t bitoffset; - long long bitvalue; - int byte, bit, on; + int byte, bit; + int byteval, bitval; + long on; if (getBitOffsetFromArgument(c,c->argv[2],&bitoffset) != REDIS_OK) return; - if (getLongLongFromObjectOrReply(c,c->argv[3],&bitvalue,err) != REDIS_OK) + if (getLongFromObjectOrReply(c,c->argv[3],&on,err) != REDIS_OK) return; - /* A bit can only be set to be on or off... */ - if (bitvalue & ~1) { + /* Bits can only be set or cleared... */ + if (on & ~1) { addReplyError(c,err); return; } @@ -142,23 +143,30 @@ void setbitCommand(redisClient *c) { } } + /* Grow sds value to the right length if necessary */ byte = bitoffset >> 3; - bit = 7 - (bitoffset & 0x7); - on = bitvalue & 0x1; o->ptr = sdsgrowzero(o->ptr,byte+1); - ((char*)o->ptr)[byte] |= on << bit; - ((char*)o->ptr)[byte] &= ~((!on) << bit); + /* Get current values */ + byteval = ((char*)o->ptr)[byte]; + bit = 7 - (bitoffset & 0x7); + bitval = byteval & (1 << bit); + + /* Update byte with new bit value and return original value */ + byteval &= ~(1 << bit); + byteval |= ((on & 0x1) << bit); + ((char*)o->ptr)[byte] = byteval; touchWatchedKey(c->db,c->argv[1]); server.dirty++; - addReply(c,shared.cone); + addReply(c, bitval ? shared.cone : shared.czero); } void getbitCommand(redisClient *c) { robj *o; - size_t bitoffset, byte, bitmask; - int on = 0; char llbuf[32]; + size_t bitoffset; + size_t byte, bit; + size_t bitval = 0; if (getBitOffsetFromArgument(c,c->argv[2],&bitoffset) != REDIS_OK) return; @@ -167,15 +175,16 @@ void getbitCommand(redisClient *c) { checkType(c,o,REDIS_STRING)) return; byte = bitoffset >> 3; - bitmask = 1 << (7 - (bitoffset & 0x7)); + bit = 7 - (bitoffset & 0x7); if (o->encoding != REDIS_ENCODING_RAW) { if (byte < (size_t)ll2string(llbuf,sizeof(llbuf),(long)o->ptr)) - on = llbuf[byte] & bitmask; + bitval = llbuf[byte] & (1 << bit); } else { if (byte < sdslen(o->ptr)) - on = ((sds)o->ptr)[byte] & bitmask; + bitval = ((char*)o->ptr)[byte] & (1 << bit); } - addReply(c, on ? shared.cone : shared.czero); + + addReply(c, bitval ? shared.cone : shared.czero); } void setrangeCommand(redisClient *c) { diff --git a/tests/unit/basic.tcl b/tests/unit/basic.tcl index 7d5667726..901507964 100644 --- a/tests/unit/basic.tcl +++ b/tests/unit/basic.tcl @@ -377,29 +377,29 @@ start_server {tags {"basic"}} { test "SETBIT against non-existing key" { r del mykey - - # Setting 2nd bit to on is integer 64, ascii "@" - assert_equal 1 [r setbit mykey 1 1] - assert_equal "@" [r get mykey] + assert_equal 0 [r setbit mykey 1 1] + assert_equal [binary format B* 01000000] [r get mykey] } test "SETBIT against string-encoded key" { - # Single byte with 2nd bit set + # Ascii "@" is integer 64 = 01 00 00 00 r set mykey "@" - # 64 + 32 = 96 => ascii "`" (backtick) - assert_equal 1 [r setbit mykey 2 1] - assert_equal "`" [r get mykey] + assert_equal 0 [r setbit mykey 2 1] + assert_equal [binary format B* 01100000] [r get mykey] + assert_equal 1 [r setbit mykey 1 0] + assert_equal [binary format B* 00100000] [r get mykey] } test "SETBIT against integer-encoded key" { + # Ascii "1" is integer 49 = 00 11 00 01 r set mykey 1 assert_encoding int mykey - # Ascii "1" is integer 49 = 00 11 00 01 - # Setting 7th bit = 51 => ascii "3" - assert_equal 1 [r setbit mykey 6 1] - assert_equal "3" [r get mykey] + assert_equal 0 [r setbit mykey 6 1] + assert_equal [binary format B* 00110011] [r get mykey] + assert_equal 1 [r setbit mykey 2 0] + assert_equal [binary format B* 00010011] [r get mykey] } test "SETBIT against key with wrong type" { From e983cf34be97ac01448f331b9c106f8b9ce55889 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 15 Dec 2010 11:20:54 +0100 Subject: [PATCH 2/6] Add fuzzy test for SETBIT --- tests/unit/basic.tcl | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/unit/basic.tcl b/tests/unit/basic.tcl index 901507964..884522a83 100644 --- a/tests/unit/basic.tcl +++ b/tests/unit/basic.tcl @@ -422,6 +422,24 @@ start_server {tags {"basic"}} { assert_error "*out of range*" {r setbit mykey 0 20} } + test "SETBIT fuzzing" { + set str "" + set len [expr 256*8] + r del mykey + + for {set i 0} {$i < 2000} {incr i} { + set bitnum [randomInt $len] + set bitval [randomInt 2] + set fmt [format "%%-%ds%%d%%-s" $bitnum] + set head [string range $str 0 $bitnum-1] + set tail [string range $str $bitnum+1 end] + set str [string map {" " 0} [format $fmt $head $bitval $tail]] + + r setbit mykey $bitnum $bitval + assert_equal [binary format B* $str] [r get mykey] + } + } + test "GETBIT against non-existing key" { r del mykey assert_equal 0 [r getbit mykey 0] From 8f8eeffec1fe1c0d40b6d078121661d7cfb02c08 Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 15 Dec 2010 11:30:50 +0100 Subject: [PATCH 3/6] Disable negative offsets for SETRANGE --- src/t_string.c | 17 +++++------------ tests/unit/basic.tcl | 34 ++-------------------------------- 2 files changed, 7 insertions(+), 44 deletions(-) diff --git a/src/t_string.c b/src/t_string.c index f1bab4dad..324e01ab4 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -195,11 +195,13 @@ void setrangeCommand(redisClient *c) { if (getLongFromObjectOrReply(c,c->argv[2],&offset,NULL) != REDIS_OK) return; + if (offset < 0) { + addReplyError(c,"offset is out of range"); + return; + } + o = lookupKeyWrite(c->db,c->argv[1]); if (o == NULL) { - /* Negative offset is always 0 for non-existing keys */ - if (offset < 0) offset = 0; - /* Return 0 when setting nothing on a non-existing string */ if (sdslen(value) == 0) { addReply(c,shared.czero); @@ -233,15 +235,6 @@ void setrangeCommand(redisClient *c) { return; } - /* Convert negative indexes. Note that for SETRANGE, the meaning of a - * negative index is a little different than for other commands. - * Here, an offset of -1 points to the trailing NULL byte of the - * string instead of the last character. */ - if (offset < 0) { - offset = olen+1+offset; - if (offset < 0) offset = 0; - } - /* Return when the resulting string exceeds allowed size */ if (checkStringLength(c,offset+sdslen(value)) != REDIS_OK) return; diff --git a/tests/unit/basic.tcl b/tests/unit/basic.tcl index 884522a83..a9b25fcec 100644 --- a/tests/unit/basic.tcl +++ b/tests/unit/basic.tcl @@ -489,14 +489,6 @@ start_server {tags {"basic"}} { r del mykey assert_equal 4 [r setrange mykey 1 foo] assert_equal "\000foo" [r get mykey] - - r del mykey - assert_equal 3 [r setrange mykey -1 foo] - assert_equal "foo" [r get mykey] - - r del mykey - assert_equal 3 [r setrange mykey -100 foo] - assert_equal "foo" [r get mykey] } test "SETRANGE against string-encoded key" { @@ -512,18 +504,6 @@ start_server {tags {"basic"}} { assert_equal 3 [r setrange mykey 1 b] assert_equal "fbo" [r get mykey] - r set mykey "foo" - assert_equal 6 [r setrange mykey -1 bar] - assert_equal "foobar" [r get mykey] - - r set mykey "foo" - assert_equal 5 [r setrange mykey -2 bar] - assert_equal "fobar" [r get mykey] - - r set mykey "foo" - assert_equal 3 [r setrange mykey -20 bar] - assert_equal "bar" [r get mykey] - r set mykey "foo" assert_equal 7 [r setrange mykey 4 bar] assert_equal "foo\000bar" [r get mykey] @@ -549,18 +529,6 @@ start_server {tags {"basic"}} { assert_encoding raw mykey assert_equal 1334 [r get mykey] - r set mykey 1234 - assert_encoding int mykey - assert_equal 5 [r setrange mykey -1 5] - assert_encoding raw mykey - assert_equal 12345 [r get mykey] - - r set mykey 1234 - assert_encoding int mykey - assert_equal 4 [r setrange mykey -2 5] - assert_encoding raw mykey - assert_equal 1235 [r get mykey] - r set mykey 1234 assert_encoding int mykey assert_equal 6 [r setrange mykey 5 2] @@ -577,7 +545,9 @@ start_server {tags {"basic"}} { test "SETRANGE with out of range offset" { r del mykey assert_error "*maximum allowed size*" {r setrange mykey [expr 512*1024*1024-4] world} + r set mykey "hello" + assert_error "*out of range*" {r setrange mykey -1 world} assert_error "*maximum allowed size*" {r setrange mykey [expr 512*1024*1024-4] world} } From 1333f98dd2ff2aae4e4c2a9a43d7b83a0b1af8aa Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 15 Dec 2010 11:40:36 +0100 Subject: [PATCH 4/6] Use helper functions in APPEND --- src/t_string.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/t_string.c b/src/t_string.c index 324e01ab4..e50692183 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -393,31 +393,26 @@ void decrbyCommand(redisClient *c) { } void appendCommand(redisClient *c) { - int retval; size_t totlen; robj *o, *append; o = lookupKeyWrite(c->db,c->argv[1]); - c->argv[2] = tryObjectEncoding(c->argv[2]); if (o == NULL) { /* Create the key */ - retval = dbAdd(c->db,c->argv[1],c->argv[2]); + c->argv[2] = tryObjectEncoding(c->argv[2]); + dbAdd(c->db,c->argv[1],c->argv[2]); incrRefCount(c->argv[2]); totlen = stringObjectLen(c->argv[2]); } else { - if (o->type != REDIS_STRING) { - addReply(c,shared.wrongtypeerr); + /* Key exists, check type */ + if (checkType(c,o,REDIS_STRING)) return; - } - append = getDecodedObject(c->argv[2]); - if (o->encoding == REDIS_ENCODING_RAW && - (sdslen(o->ptr) + sdslen(append->ptr)) > 512*1024*1024) - { - addReplyError(c,"string exceeds maximum allowed size (512MB)"); - decrRefCount(append); + /* "append" is an argument, so always an sds */ + append = c->argv[2]; + totlen = stringObjectLen(o)+sdslen(append->ptr); + if (checkStringLength(c,totlen) != REDIS_OK) return; - } /* If the object is shared or encoded, we have to make a copy */ if (o->refcount != 1 || o->encoding != REDIS_ENCODING_RAW) { @@ -429,7 +424,6 @@ void appendCommand(redisClient *c) { /* Append the value */ o->ptr = sdscatlen(o->ptr,append->ptr,sdslen(append->ptr)); - decrRefCount(append); totlen = sdslen(o->ptr); } touchWatchedKey(c->db,c->argv[1]); From ad1b4f4f59e653ac310ee0afb847daeb4f19fbfd Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 15 Dec 2010 11:49:04 +0100 Subject: [PATCH 5/6] Use helper function for string object length --- src/t_string.c | 23 +++-------------------- 1 file changed, 3 insertions(+), 20 deletions(-) diff --git a/src/t_string.c b/src/t_string.c index e50692183..eb080c882 100644 --- a/src/t_string.c +++ b/src/t_string.c @@ -215,21 +215,14 @@ void setrangeCommand(redisClient *c) { o = createObject(REDIS_STRING,sdsempty()); dbAdd(c->db,c->argv[1],o); } else { - int olen; + size_t olen; /* Key exists, check type */ if (checkType(c,o,REDIS_STRING)) return; - /* Find out existing value length */ - if (o->encoding == REDIS_ENCODING_INT) { - char llbuf[32]; - olen = ll2string(llbuf,sizeof(llbuf),(long)o->ptr); - } else { - olen = sdslen(o->ptr); - } - /* Return existing string length when setting nothing */ + olen = stringObjectLen(o); if (sdslen(value) == 0) { addReplyLongLong(c,olen); return; @@ -433,18 +426,8 @@ void appendCommand(redisClient *c) { void strlenCommand(redisClient *c) { robj *o; - if ((o = lookupKeyReadOrReply(c,c->argv[1],shared.czero)) == NULL || checkType(c,o,REDIS_STRING)) return; - - if (o->encoding == REDIS_ENCODING_RAW) { - addReplyLongLong(c,sdslen(o->ptr)); - } else if (o->encoding == REDIS_ENCODING_INT) { - char llbuf[32]; - int len = ll2string(llbuf,sizeof(llbuf),(long)o->ptr); - addReplyLongLong(c,len); - } else { - redisPanic("Unknown string encoding"); - } + addReplyLongLong(c,stringObjectLen(o)); } From 7d5f5712d948b19d0778e4ef53a27d213aff7bdc Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Wed, 15 Dec 2010 11:49:39 +0100 Subject: [PATCH 6/6] Update tests for STRLEN --- tests/unit/basic.tcl | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/unit/basic.tcl b/tests/unit/basic.tcl index a9b25fcec..5098b0109 100644 --- a/tests/unit/basic.tcl +++ b/tests/unit/basic.tcl @@ -361,18 +361,18 @@ start_server {tags {"basic"}} { list [r msetnx x1 xxx y2 yyy] [r get x1] [r get y2] } {1 xxx yyy} - test {STRLEN against non existing key} { - r strlen notakey - } {0} + test "STRLEN against non-existing key" { + assert_equal 0 [r strlen notakey] + } - test {STRLEN against integer} { + test "STRLEN against integer-encoded value" { r set myinteger -555 - r strlen myinteger - } {4} + assert_equal 4 [r strlen myinteger] + } - test {STRLEN against plain string} { + test "STRLEN against plain string" { r set mystring "foozzz0123456789 baz" - r strlen mystring + assert_equal 20 [r strlen mystring] } test "SETBIT against non-existing key" {