Send and receive data code

Question

I need to send and receive data. Here is solution for my original problem. I want to know how safe my finished code is and how I can improve it.

int sslWrite(SSL *ssl, const std::string &data)
{
    return sslWrite(ssl, &data[0], data.size());
}

int sslWrite(SSL *ssl, const void *buf, unsigned num)
{
    const int INT_SIZE = sizeof(int);
    const int numv = INT_SIZE + num;

    std::vector<char> bufv(numv);

    memcpy(&bufv[0], &numv, INT_SIZE);
    if (num != 0) {
        memcpy(&bufv[INT_SIZE], buf, num);
    }

    int n = 0;
    while (n < num) {
        int w = SSL_write(ssl, &bufv[0], numv - n);
        if (w < 0) {
            return 0;
        }
        n += w;
    }

    return n;
}

const int BUF_INIT_SIZE = 0x10000;

std::vector<char> sslRead(SSL *ssl)
{
    const int INT_SIZE = sizeof(int);
    std::vector<char> bufv(BUF_INIT_SIZE);

    int m = SSL_read(ssl, &bufv.at(0), BUF_INIT_SIZE);

    if (m == 0) {
        return std::vector<char>();
    }

    if (m < 0) {
        throw std::runtime_error("m < 0");
    }

    const int n = *reinterpret_cast<int *>(&bufv[0]);
    bufv.resize(n);

    while (m < n) {
        int k = SSL_read(ssl, &bufv.at(m), n - m);
        if (k < 0) {
            throw std::runtime_error("k < 0");
        }
        m += k;
    }

    bufv.resize(m);

    if (m < INT_SIZE) {
        throw std::runtime_error("m < INT_SIZE");
    }

    bufv.erase(bufv.begin(), bufv.begin() + INT_SIZE);
    return bufv;
}

Answer 1

• Portability

  The code would fail if the sender and receiver platforms disagree on the size of integer.

  The code would fail if the sender and receiver platforms disagree on the endianness of integer.

• Partial reads

  There is a chance that the very first read returns just a couple of bytes. The code still interprets it as a valid integer.

• Overall

  There is no need to use the same buffer for both size and payload.

  m,n,k are patently bad variable names.

• Recommendations

  Implement a separate send_size() and read_size() routines.

Answer 2

Here are some more problems (on top of what @vnp wrote):

• the exception messages you added, should (realy really) change; What should a client of your library do if they called your API and got an exception with the message "k < 0"? A client should only need to know the contract of your API: preconditions, postconditions, inputs, outputs and side effects (not what you used a variable named "k" for, and why it should have positive values).

• the reading function ignores the complexity of the buffering problem.

  Consider a situation where sslWrite is called in a loop, and you have five messages, buffered at the other side of the connection, before you call sslRead.

  In this case, sslRead will read BUF_INIT_SIZE = 0x10000 characters (let's say that means "three and a half messages"), then process the first message, and the rest will be lost in the read buffer, when you exit the function;

  Because of this, the buffer used for reading should be processed in a loop.

  In the case when at the end of the loop, you have part of the next message in the buffer, this buffer should be cached until the next read (otherwise, at the next call of sslRead, you will interpret four (or eight) bytes from the middle of the message as an integer length, and get UB from that point onwards.

• this line in sslWrite is wrong:

  int w = SSL_write(ssl, &bufv[0], numv - n);
  

  In the case when the message cannot be written in a single loop, the function will printthe first part of the message as the second part.

  To correct it, you should either dereference &buf[n] (instead of &buf[0]), or add a line like bufv.erase(bufv.begin(), bufv.begin() + n); dirrectly under the SSL_write call.