search

Jdesk / memcached

Automatically exported from code.google.com/p/memcached
0 stars 0 forks source link

Binary Increment returns old cas #220

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
What steps will reproduce the problem?
1. Send the following byte stream (which is interpreted by Memcached to be SET 
where key="Bob" and val="100" 
0x80 0x01 0x00 0x03 
0x08 0x00 0x00 0x00 
0x00 0x00 0x00 0x0e 
0xde 0xad 0xbe 0xef 
0x00 0x00 0x00 0x00 
0x00 0x00 0x00 0x00 
0xc8 0x11 0x00 0x00 
0x00 0x00 0x00 0x00 
0x42 0x6f 0x62 0x31 
0x30 0x30 

2. Send the Increment with increment size = 999
0x80 0x01 0x00 0x03 
0x08 0x00 0x00 0x00 
0x00 0x00 0x00 0x0e 
0xde 0xad 0xbe 0xef 
0x00 0x00 0x00 0x00 
0x00 0x00 0x00 0x00 
0xc8 0x11 0x00 0x00 
0x00 0x00 0x00 0x00 
0x42 0x6f 0x62 0x31 
0x30 0x30

What is the expected output? What do you see instead?
I expect the cas to be incremented; instead I see the old cas. However, when I 
do the next get, it returns the new, correct cas. 

==Expected byte stream==
0x81 0x05 0x00 0x00 
0x00 0x00 0x00 0x00 
0x00 0x00 0x00 0x08 
0xde 0xad 0xbe 0xef 
0x00 0x00 0x00 0x00 
0x00 0x00 0x00 0x02 
0x00 0x00 0x00 0x00 
0x00 0x00 0x04 0x4b 

==Observed byte stream==
0x81 0x05 0x00 0x00 
0x00 0x00 0x00 0x00 
0x00 0x00 0x00 0x08 
0xde 0xad 0xbe 0xef 
0x00 0x00 0x00 0x00 
0x00 0x00 0x00 0x01 
0x00 0x00 0x00 0x00 
0x00 0x00 0x04 0x4b 

What version of the product are you using? On what operating system?
memcached 1.4.5 on Ubuntu 11.04 x64

Please provide any additional information below.
I've looked at the source code, and it appears that the problem resides in how 
the logic in do_add_delta will give the the new cas to new_it and the function 
that sends the response, complete_incr_bin, will read it from the original 
item, it.

Original issue reported on code.google.com by step...@uci.edu on 3 Sep 2011 at 1:20

GoogleCodeExporter commented 9 years ago 
Can you try to reproduce this under 1.4.7? I believe this may have been fixed.

Original comment by dorma...@rydia.net on 3 Sep 2011 at 1:24

GoogleCodeExporter commented 9 years ago 
I've tried it under 1.4.7 and it still fails. From lines 2903 to 2906 in 
memcached.c is where the the problem is. I think somewhere after line 2903, you 
need this block of code:

    if (cas) {
        *cas = ITEM_get_cas(new_it); 
    }

Original comment by step...@uci.edu on 3 Sep 2011 at 2:35

GoogleCodeExporter commented 9 years ago 
I haven't written a test, but I don't see this bug in 1.4.7 or newer. That 
exact block of code was added to the end of do_add_delta, so there's no way for 
the old cas to persist past do_add_delta.

Unless I messed up the reference in there, but I don't see it.

Unfortunately the perl tests don't test incrdecr's cas return, so I'll have to 
add support for that before closing the bug :/

Are you *absolutely sure* you used 1.4.7 in your latest test?

Original comment by dorma...@rydia.net on 28 Sep 2011 at 5:09

GoogleCodeExporter commented 9 years ago 
I tested this using the 1.4.7 Aug 16 release and the bug is there. 

-=Bug Explanation=-
The reason why that exact block of code won't work at the end of do_add_delta 
is because it's getting the return cas from "it". The new cas that should be 
returned is assigned to "new_it" in "item_replace(it, new_it)". Two possible 
fixes would either be:
1) insert logic in item_replace to copy over the cas from the "new_it" to "it" 
or
2) in do_add_delta, put the block of code I've typed in comment 2 after line 
2906 in do_add_delta and return OK. 

-=Easy Way to Reproduce Bug=-
I've also found an easy way to replicate the error:
1) startup memcached with verbose mode "memcached -vvv"
2) send custom packet to set the key "Bob" with value "100" using the following 
command in Ubuntu:

echo -en 
'\x80\x01\x00\x03\x08\x00\x00\x00\x00\x00\x00\x0e\xde\xad\xbe\xef\x00\x00\x00\x0
0\x00\x00\x00\x00\xc8\x11\x00\x00\x00\x00\x00\x00\x42\x6f\x62\x31\x30\x30' | nc 
localhost 11211

2.5) Take note of the CAS ID in the response header by looking at the verbose 
output. 

3) Send packet to increment "Bob" by 999 using the command: 

echo -en 
'\x80\x05\x00\x03\x14\x00\x00\x00\x00\x00\x00\x17\xde\xad\xbe\xef\x00\x00\x00\x0
0\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\xE7\x00\x00\x00\x00\x00\x00\x00\x0
1\x00\x00\x00\x00\x42\x6f\x62' | nc localhost 11211

4) Note that the CAS in the return packet is the same as the one in step 2.5 
(even if you had other successful set/add/replace commands between steps 2 and 
3). This means that the old cas is being returned. 

I hope the extra info helps.

Original comment by step...@uci.edu on 28 Sep 2011 at 7:35

GoogleCodeExporter commented 9 years ago 
Easier repro:

>>> import mc_bin_client
>>> c = mc_bin_client.MemcachedClient()
>>> c.set('a', 0, 0, '100')
(2913549530, 1, '')
>>> c.incr('a', 999)
(1099, 1)
>>> c.incr('a', 999)
(2098, 3)
>>> c.incr('a', 999)
(3097, 4)

I don't see that in the engine branch, but I do see it in 1.4.7-2-g51c8f31.  
I'll put together a fix real quick.

Original comment by dsalli...@gmail.com on 28 Sep 2011 at 7:42

GoogleCodeExporter commented 9 years ago 
ah, I see it now. It's much easier to understand when my roommate isn't whining 
about something.

I'll give dustin a chance to fix it though :)

Original comment by dorma...@rydia.net on 28 Sep 2011 at 7:46

GoogleCodeExporter commented 9 years ago 
I've got a fix up here:  https://github.com/dustin/memcached/tree/bug220

Dormando/Trond, please review and grab it or tell me what to do to make it 
better.

Original comment by dsalli...@gmail.com on 28 Sep 2011 at 8:09

GoogleCodeExporter commented 9 years ago 
Looks fine here. Merged this into my for_148 branch. Thanks!

Closing 'deh boog.

Original comment by dorma...@rydia.net on 28 Sep 2011 at 8:29