My favorites | Sign in
Project Home Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 102: PIping null to the server will crash it
1 person starred this issue and may be notified of changes. Back to list
Status:  Fixed
Owner:  trond.no...@gmail.com
Closed:  Oct 2009


Sign in to add a comment
 
Reported by fallenpe...@gmail.com, Oct 27, 2009
What steps will reproduce the problem?
1. do "cat /dev/zero | nc -q1 127.0.0.1 11211"
2. Wait a short while
3. Watch the server crash

What is the expected output? What do you see instead?

The server should probably disconnect the client on such bad input

What version of the product are you using? On what operating system?

memcached 1.2.8

ubuntu jaunty

Please provide any additional information below.


Oct 28, 2009
Project Member #1 trond.no...@gmail.com
The problem here is that the client will try to spool the data it receives from the
client until it receives '\n' (indicating that it got the complete command). In this
case we never receive '\n' so we end up reallocating the input buffer until realloc
fails, causing the client connection to be shut down. My server didn't crash, but
that could be caused other mallocs succeeding (and failing in your case). There might
therefore be a path through the code that doesn't handle a malloc failure.

To protect us against someone eating up all of the memory in one buffer, I suggest
that we just shut down the connection if we receive more than KEY_MAX_LENGTH + let's
say 100 bytes (command, flags, length, cas etc) without seeing '\n'. 

Another part of the problem is try_read_network. in this function we try to read out
all of the data from the socket reallocating the input buffer to a bigger buffer if
there is more data available. In the test-case above we might never break out of this
loop, because one core can fill the buffer while the other core on the system is busy
reading and reallocating. As a workaround for this we should jump back out after a
low number of reallocs.

Proposed fix at: http://github.com/trondn/memcached/tree/issue_102
Status: Started
Owner: trond.norbye
Oct 28, 2009
Project Member #2 dsalli...@gmail.com
This doesn't seem right.  Doesn't that make multiget not work, or am I not understanding?

I think the first, say, 16 bytes needs to look like a valid command at the very
least.  Your code makes sense for anything except get, so perhaps it'd be OK and get
can be special cased?

It'd also be good to do away with the "allow any arbitrary whitespace before
commands" stuff in the protocol.  That'd simplify it down to just checking the first
byte for most things.

Oct 28, 2009
Project Member #3 dsalli...@gmail.com
Latest changes look good.  Thanks!  :)
Status: Fixed
Sign in to add a comment

Powered by Google Project Hosting