binary protocol incr on text returns success 0

[GoogleCodeExporter]

With memcached 1.3.x, for the binary protocol,
incrementing/decrementing a non-number incorrectly gives you a success
response and 0 (numeric 0) value.  In the ascii protocol, for
comparison, you get an error "CLIENT_ERROR cannot increment or
decrement non-numeric value".

It looks like the complete_incr_bin() function in memcached.c that
calls add_delta() is ignoring add_delta()'s return value, and also
looks like it's using an uninitialized stack buffer.

Using Dustin's python test framework
(http://github.com/dustin/memcached-test)...

    def testIncrNonNumber(self):
        """Test incrementing a non-number value."""
        self.mc.set("x", 0, 0, "text")
        val, cas = self.mc.incr("x")
        print(val) # This prints 0 rather than raising an error.

Partial output from running memcached in -vvv mode...

30: going from conn_read to conn_parse_cmd
<30 Read binary protocol data:
<30    0x80 0x05 0x00 0x01
<30    0x14 0x00 0x00 0x00
<30    0x00 0x00 0x00 0x15
<30    0x00 0x00 0x00 0x00
<30    0x00 0x00 0x00 0x00
<30    0x00 0x00 0x00 0x00
30: going from conn_parse_cmd to conn_nread
incr x 1, 0, -1
> FOUND KEY x
>30 Writing bin response:
>30   0x81 0x05 0x00 0x00
>30   0x00 0x00 0x00 0x00
>30   0x00 0x00 0x00 0x08
>30   0x00 0x00 0x00 0x00
>30   0x00 0x00 0x00 0x00
>30   0x00 0x00 0x00 0x09

Original issue reported on code.google.com by steve....@gmail.com on 13 May 2009 at 12:21

[GoogleCodeExporter]

I've a test and stuff for this.  Waiting for some feedback for what exactly to 
do in
the error case.

Original comment by dsalli...@gmail.com on 21 Jun 2009 at 7:13

• Changed state: Accepted

[GoogleCodeExporter]

This is going to be a 1.4 blocker just because it's a protocol specification 
whole
that needs to be filled before the first real one ships.

It's a small hole, but should be covered.

Original comment by dsalli...@gmail.com on 25 Jun 2009 at 10:43

• Changed state: Started
• Added labels: Priority-High
• Removed labels: Priority-Medium

[GoogleCodeExporter]

I defined a new error for this.  My tree is up here:

http://github.com/dustin/memcached/commits/issue48

It's broken down into two parts:

  1) Quick fix to get the right response delivered.
  2) Fixing add_delta to return status instead of text protocol.

Passes all tests on all platforms (except solaris/sparc, which is currently 
down).

Original comment by dsalli...@gmail.com on 28 Jun 2009 at 1:10

[GoogleCodeExporter]

Fix is pushed.  Thanks, Trond!

Original comment by dsalli...@gmail.com on 29 Jun 2009 at 9:27

• Changed state: Fixed

[GoogleCodeExporter]

i'm not sure which version includes this fix, but both 1.4.3 and 1.4.4 give me 
a status code 0 (SUCCESS) when 
trying to increment non numeric values:

(MyKey has the value "Hello World")

incr MyKey 12, 1, 0
>24 Writing an error: Non-numeric server-side value for incr or decr
>24 Writing bin response:
>24   0x81 0x06 0x00 0x00
>24   0x00 0x00 0x00 0x06
>24   0x00 0x00 0x00 0x2e
>24   0x00 0x00 0x00 0x05
>24   0x00 0x00 0x00 0x00
>24   0x00 0x00 0x00 0x00

Original comment by a%enyim....@gtempaccount.com on 5 Mar 2010 at 9:32

[GoogleCodeExporter]

I see status code 6 and the error text there.  6 == bad value for incr/decr.  I 
believe this 
is correct.  This change went in as 1.4.0-rc1-2-gcce46e8

Original comment by dsalli...@gmail.com on 6 Mar 2010 at 12:03

[GoogleCodeExporter]

yeah, sorry i forgot that the status is supposed to be 16bit not 8.

Original comment by a%enyim....@gtempaccount.com on 6 Mar 2010 at 12:07