memached 1.6 - write_bin_packet will call add_iov with pointer to a freed stack variable

[GoogleCodeExporter]

From reading of 
https://github.com/memcached/memcached/commit/eeaeeede5ddaaed8179389876866756ce1
c05158 , it seems that write_bin_packet will write to a stack based buffer, and 
add a pointer to it for later delivery using add_iov.
As the actual transmission happens later on, this will cause the writing 
function to send uninitialized buffer, containing random stack contents.
The bug was introduced in eeaeeed , with versions before it passing pointers to 
constant strings, that were not stored on the stack.

Original issue reported on code.google.com by shach...@gmail.com on 19 Dec 2012 at 12:08

[GoogleCodeExporter]

I haven't reviewed this closely, but the iov stuff for binprot responses is 
used immediately. That's probably why it currently works.

I don't think write_bin_packet is a function? but I think i know what you were 
referring to.

If you can provide a test showing it returning random data instead of the 
intended data?

Original comment by dorma...@rydia.net on 23 Dec 2012 at 2:29

[GoogleCodeExporter]

Here is the code of add_iov:
/*
 * Adds data to the list of pending data that will be written out to a
 * connection.
 *
 * Returns 0 on success, -1 on out-of-memory.
 */

static int add_iov(conn *c, const void *buf, int len) {
    struct msghdr *m;
    int leftover;
    bool limit_to_mtu;

    assert(c != NULL);

    do {
        m = &c->msglist[c->msgused - 1];

        /*
         * Limit UDP packets, and the first payloads of TCP replies, to
         * UDP_MAX_PAYLOAD_SIZE bytes.
         */
        limit_to_mtu = IS_UDP(c->transport) || (1 == c->msgused);

        /* We may need to start a new msghdr if this one is full. */
        if (m->msg_iovlen == IOV_MAX ||
            (limit_to_mtu && c->msgbytes >= UDP_MAX_PAYLOAD_SIZE)) {
            add_msghdr(c);
            m = &c->msglist[c->msgused - 1];
        }

        if (ensure_iov_space(c) != 0)
            return -1;

        /* If the fragment is too big to fit in the datagram, split it up */
        if (limit_to_mtu && len + c->msgbytes > UDP_MAX_PAYLOAD_SIZE) {
            leftover = len + c->msgbytes - UDP_MAX_PAYLOAD_SIZE;
            len -= leftover;
        } else {
            leftover = 0;
        }

        m = &c->msglist[c->msgused - 1];
        m->msg_iov[m->msg_iovlen].iov_base = (void *)buf;
        m->msg_iov[m->msg_iovlen].iov_len = len;

        c->msgbytes += len;
        c->iovused++;
        m->msg_iovlen++;

        buf = ((char *)buf) + len;
        len = leftover;
    } while (leftover > 0);

    return 0;
}

As you can see, it stores a pointer to the buffer, not sending the buffer yet. 
Sending the buffer will only happen once the connection is ready for writing.

I don't have a testcase available at hand. Can you point me to existing binary 
protocol test cases, that I can start the development of the testcase from?

--Shachar

Original comment by shach...@gmail.com on 6 Jan 2013 at 1:57

[GoogleCodeExporter]

testapp.c and t/binary.t

Original comment by dorma...@rydia.net on 6 Jan 2013 at 7:06

[GoogleCodeExporter]

Original comment by dsalli...@gmail.com on 12 Feb 2013 at 6:59