Export to GitHub

memcached - issue #306

Crash when performing deletion


Posted on Jan 8, 2013 by Grumpy Dog

1) Run memcached 1.4.4 on RHEL6 and pass "-vv" 2) Send binary deletion requests.

memcached will print the key being deleted to stderr (file: memcached.c, function: process_bin_delete, ll. 2002 ff.):

if (settings.verbose > 1) {
    fprintf(stderr, "Deleting %s\n", key);
}

Since the key is not NUL-terminated this may run off the end of the connexion object's read buffer and cause a seg-fault. Compare the code for printing the key elsewhere (file: memcached.c, function process_bin_update, ll. 1842 ff.):

if (settings.verbose > 1) {
    int ii;
    if (c->cmd == PROTOCOL_BINARY_CMD_ADD) {
        fprintf(stderr, "<%d ADD ", c->sfd);
    } else if (c->cmd == PROTOCOL_BINARY_CMD_SET) {
        fprintf(stderr, "<%d SET ", c->sfd);
    } else {
        fprintf(stderr, "<%d REPLACE ", c->sfd);
    }
    for (ii = 0; ii < nkey; ++ii) {
        fprintf(stderr, "%c", key[ii]);
    }

    fprintf(stderr, " Value len is %d", vlen);
    fprintf(stderr, "\n");
}

Comment #1

Posted on Jan 8, 2013 by Helpful Bird

We don't support 1.4.4. Can you reproduce this with a newer version?

Comment #2

Posted on Jan 9, 2013 by Grumpy Dog

Checked out the current master (9e09900770e79e4e621bdd274658dfa748404095), disabled setrlimit (RLIMIT_NOFILE, ...) 'cause it doesn't seem to play well with valgrind, compiled and installed it on my Debian Testing machine (build log attached).

Started it as follows:

$ valgrind --leak-check=full --malloc-fill=0xee --free-fill=0xff --trace-children=yes --log-file=$TMPDIR/memcached.vg.%p.log /usr/local/memcached-9e09900/bin/memcached -vv -p 2300 2>$TMPDIR/memcached.log & [1] 19335

Attempted to remove two keys as follows:

$ memrm --servers localhost:2300 --binary ABCDEF xyz memrm: ABCDEF: memcache error NOT FOUND memrm: xyz: memcache error NOT FOUND

Valgrind and memcached logs are attached.

To summarize, the key is not NUL-terminated and the fprintf may run off the end of the end of the buffer.

Attachments

Comment #3

Posted on Jan 10, 2013 by Helpful Bird

Thanks! Just wanted confirmation with the latest code before I dug into it.

Comment #4

Posted on Jan 10, 2013 by Grumpy Dog

Understood. I've attached the patch I wrote during testing.

Attachments

Comment #5

Posted on Jan 15, 2013 by Quick Wombat

will this patch be committed as is?

Comment #6

Posted on Jan 15, 2013 by Grumpy Dog

Found another instance of this in items.c, do_item_get, ll. 539ff.:

if (settings.verbose > 2) {
    if (it == NULL) {
        fprintf(stderr, "> NOT FOUND %s", key);
    } else {
        fprintf(stderr, "> FOUND KEY %s", ITEM_key(it));
        was_found++;
    }
}

Here's a valgrind stack-trace:

==22568== Conditional jump or move depends on uninitialised value(s)
==22568==    at 0x30F78478DE: vfprintf (in /lib64/libc-2.12.so)
==22568==    by 0x30F784948F: buffered_vfprintf (in /lib64/libc-2.12.so)
==22568==    by 0x30F784449D: vfprintf (in /lib64/libc-2.12.so)
==22568==    by 0x30F784EF97: fprintf (in /lib64/libc-2.12.so)
==22568==    by 0x40EC3D: do_item_get (items.c:541)
==22568==    by 0x410B35: item_get (thread.c:499)
==22568==    by 0x408897: complete_nread_binary (memcached.c:1303)
==22568==    by 0x40B07F: event_handler (memcached.c:2256)
==22568==    by 0x30F7C06B43: event_base_loop (in /usr/lib64/libevent-1.4.so.2.1.3)
==22568==    by 0x4101DC: worker_libevent (thread.c:384)
==22568==    by 0x30F8007850: start_thread (in /lib64/libpthread-2.12.so)
==22568==    by 0x663E6FF: ???

Comment #7

Posted on Dec 20, 2013 by Helpful Bird

Merged your patch, and fixed two more of these instances on my own (the one you pointed out and one I quickly grepped out of the source tree).

Pushed to pending tree. Sorry for the long delay.

Status: Fixed

Labels:
Type-Defect Priority-Medium