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 BirdWe don't support 1.4.4. Can you reproduce this with a newer version?
Comment #2
Posted on Jan 9, 2013 by Grumpy DogChecked 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.
- memcached-build.log 17.26KB
- memcached.log 7.78KB
- memcached.vg.19335.log 7.6KB
Comment #3
Posted on Jan 10, 2013 by Helpful BirdThanks! Just wanted confirmation with the latest code before I dug into it.
Comment #4
Posted on Jan 10, 2013 by Grumpy DogUnderstood. I've attached the patch I wrote during testing.
Comment #5
Posted on Jan 15, 2013 by Quick Wombatwill this patch be committed as is?
Comment #6
Posted on Jan 15, 2013 by Grumpy DogFound 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 BirdMerged 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