Boost Asio incomplete write to socket

Question

I'm writing a dead simple toy key-value store using Boost Asio, and something really weird is happening.

The string-based protocol is like this:

S <key> <value> // to set a key
G <key>         // to get the key value
L               // to list all key-value pairs

The write is synchronous, using

boost::asio::write(socket_,boost::asio::buffer(resp, len));

where socket_ is a boost::asio::ip::tcp::socket - with asynchrounous writes the story does not change, apparently.

The problem is that sometimes it does not write to the socket all the bytes it is supposed to write, or the outputs is mangled somehow...

Example of mangled list output (on localhost, using nc, echo and hexdump):

> echo S a 12 | nc localhost 5000       
A
> echo S b 23 | nc localhost 5000
A
> echo L | nc localhost 5000 | hexdump -C
00000000  61 3a 20 31 32 3b 20 62  60 00 00 00 00 00        |a: 12; b`.....|
0000000e
> echo L | nc localhost 5000 | hexdump -C
00000000  61 3a 20 31 32 3b 20 62  3a 20 32 33 3b 20        |a: 12; b: 23; |
0000000e

I'm using Boost 1.55 from Ubuntu 14.10 repository. Follows the code of the functions that serves the client.

Thank you in advance for any hint!

void ClientSession::handle_read(const boost::system::error_code& error, size_t bytes_transferred) {
if (!error) {

    std::string cmd(data_, bytes_transferred);
    cmd = trim_str(cmd);

    const char* resp = NULL;
    int len = 0;

    switch(cmd.at(0)) {
    case SET: {
        std::size_t k_pos = cmd.find(" ") + 1;
        std::size_t v_pos = cmd.find(" ", k_pos+1) + 1;

        std::string key = trim_str(cmd.substr(k_pos, v_pos-3));
        std::string value = trim_str(cmd.substr(v_pos, cmd.length()-1));
        cout << "SET key " << key << ", value " << value << "*" <<endl;

        kvs->db[key] = std::atoi(value.c_str());

        resp = "A";
        len = 1;
        break;
    }
    case GET: {
        std::size_t k_pos = cmd.find(" ") + 1;
        std::string key = trim_str(cmd.substr(k_pos, cmd.length()));
        cout << "GET key " << key << "*" << endl;

        int value = kvs->db[key];
        char str[5];
        sprintf(str, "%d", value);
        resp = (const char*) str;
        len = strlen(resp);
        break;
    }
    case LIST: {
        ostringstream os;
        for (std::map<string, int>::iterator iter = kvs->db.begin();
              iter != kvs->db.end(); ++iter )
              os << iter->first << ": " << iter->second << "; ";
        cout << "list: " << os.str().c_str() << endl;

        resp = os.str().c_str();
        len = strlen(resp);
        break;
    }
    case DEL: {
        std::size_t k_pos = cmd.find(" ") + 1;
        std::string key = trim_str(cmd.substr(k_pos, cmd.length()));

        kvs->db.erase(key);
        resp = "A";
        len = 1;
        break;
    }
    default: {
        resp = "NACK.";
        len = 5;
    }
    }

    cout << "resp: " << resp << "*" << endl;
    cout << "len: " << len << "*" << endl;
    std::size_t written = boost::asio::write(socket_,
            boost::asio::buffer(resp, len));
    cout << "written: " << written << endl;

    boost::system::error_code ignored_ec;
    socket_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ignored_ec);
    socket_.close();

} else
    delete this;

Answer 1

At the very least you're having undefined behaviour because of dangling resp pointer in case GET:

{
    // ...
    char str[5];
    resp = (const char*) str; // WHOOOOOOOOOOOPS
    len = strlen(resp);
}

The exact same thing under case LIST:

{
    std::ostringstream os;
    // ....
    resp = os.str().c_str(); // WHOOOOOOOOOOOPS
}

All reasoning about the program stops to be useful when you have Undefined Behaviour.

Fix these issues (and potentially more that I didn't look for), and retest. Run under valgrind. Use static analysis tools.

Update: fixed up version for single threading: https://gist.github.com/sehe/69379e17350fb718892f#comment-1428235

Test run output:

$ for a in S{a..d}\ $RANDOM Gnonexisting L; do echo "$a -> $(netcat 127.0.0.1 5000 <<< "$a")"; done | nl
     1  Sa 15936 -> A
     2  Sb 3671 -> A
     3  Sc 10550 -> A
     4  Sd 7741 -> A
     5  Gnonexisting -> 0
     6  L -> 1: 1; 2: 2; a: 15936; asdasd: 0; b: 3671; c: 10550; d: 7741; nonexisting: 0; 

The code for the handle_read looks like:

void ClientSession::handle_read(const boost::system::error_code& error, size_t bytes_transferred) {
    if (!error) {
        std::istringstream request(std::string(data_, bytes_transferred));
        boost::asio::streambuf resp;
        std::ostream os(&resp);

        char cmd_char = 0;
        std::string key;
        int value;
        if (request >> cmd_char) switch(cmd_char) {
            case SET:                          
                if (request >> key >> value)
                    kvs->db[key] = value;

                os << "A";
                break;
            case GET:
                if (request >> key)
                    os << kvs->db[key];
                break;

            case LIST:
                for (auto const& e : kvs->db)
                    os << e.first << ": " << e.second << "; ";
                break;

            case DEL: 
                if (request >> key)
                    kvs->db.erase(key);

                os << "A";
                break;
            default: 
                os << "NACK.";
        }

        cout << "resp: " << &resp << "*" << endl;
        cout << "len: " << resp.size() << "*" << endl;
        std::size_t written = boost::asio::write(socket_, resp);
        cout << "written: " << written << endl;

        boost::system::error_code ignored_ec;
        socket_.shutdown(boost::asio::ip::tcp::socket::shutdown_both, ignored_ec);
        socket_.close();
    } else
        delete this;
}