Crash when performing deletion

[GoogleCodeExporter]

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");
    }

Original issue reported on code.google.com by jeremy.s...@gmail.com on 8 Jan 2013 at 6:23

[GoogleCodeExporter]

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

Original comment by dorma...@rydia.net on 8 Jan 2013 at 7:11

[GoogleCodeExporter]

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.

Original comment by jeremy.s...@gmail.com on 9 Jan 2013 at 11:51

Attachments:

• memcached-build.log
• memcached.log
• memcached.vg.19335.log

[GoogleCodeExporter]

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

Original comment by dorma...@rydia.net on 10 Jan 2013 at 6:55

[GoogleCodeExporter]

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

Original comment by jeremy.s...@gmail.com on 10 Jan 2013 at 10:43

Attachments:

• 0001-Fix-buffer-overrun-when-logging-key-to-delete-in-bin.patch

[GoogleCodeExporter]

will this patch be committed as is?

Original comment by noneofyo...@gmail.com on 15 Jan 2013 at 11:47

[GoogleCodeExporter]

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: ???

Original comment by jeremy.s...@gmail.com on 15 Jan 2013 at 1:19

[GoogleCodeExporter]

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.

Original comment by dorma...@rydia.net on 20 Dec 2013 at 9:29

• Changed state: Fixed