3
\$\begingroup\$

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;
}
\$\endgroup\$
0
4
\$\begingroup\$
  • 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.

\$\endgroup\$
2
  • \$\begingroup\$ What can I do for 'disagree on the endianness of integer'? | 'both size and payload' I don't understand what do you mean. | 'Implement a separate send_size() and read_size()' But in this case there will be need to call two times instead of one for most cases. Are you sure it is good idea? \$\endgroup\$ – Ufx Aug 14 '15 at 8:28
  • 1
    \$\begingroup\$ @Ufx Convert size to a predefined (preferably network) endiannes via one of hton*() calls. Making 2 calls is OK; ovehead of a function call is infinitesimal (especially compared to overhead of erasing a beginning of the buffer) \$\endgroup\$ – vnp Aug 14 '15 at 16:56
1
\$\begingroup\$

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.

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.