2
\$\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. Here is my first try. Now I need to get review for my new code.

void sslWrite(SSL *ssl, const void *buf, unsigned size)
{
    int writedTotal = 0;
    const char *ccBuf = reinterpret_cast<const char *>(buf);

    while (writedTotal < size) {
        int writedPart = SSL_write(ssl, ccBuf + writedTotal, size - writedTotal);
        if (writedPart < 0) {
            std::string error = "SSL_write error = ";
            error += (std::to_string(writedPart) + ".");
            throw std::runtime_error(error);
        }
        writedTotal += writedPart;
    }
}

std::vector<char> sslRead(SSL *ssl, unsigned size)
{
    std::vector<char> ret(size);
    unsigned readTotal = 0;

    while (readTotal < size) {
        int readPart = SSL_read(ssl, &ret[readTotal], size - readTotal);
        if (readPart < 0) {
            std::string error = "SSL_read error = ";
            error += (std::to_string(readPart) + ".");
            throw std::runtime_error(error);
        }
        readTotal += readPart;
    }

    return ret;
}

void sslWriteSize(SSL *ssl, unsigned size)
{
    if (sizeof(u_long) > 8)
        throw std::runtime_error("Write size error.");

    u_long nlSize = htonl(size);
    sslWrite(ssl, &nlSize, 8);
}

unsigned sslReadSize(SSL *ssl)
{
    if (sizeof(u_long) > 8)
        throw std::runtime_error("Read size error.");

    u_long size = *reinterpret_cast<u_long *>(&sslRead(ssl, 8)[0]);
    size = ntohl(size);
    return size;
}

void sslWrite(SSL *ssl, const std::string &data)
{
    if (data.empty())
        throw std::runtime_error("SSL write error. Data is empty.");

    sslWriteSize(ssl, data.size());
    sslWrite(ssl, &data.at(0), data.size());
}

std::vector<char> sslRead(SSL *ssl)
{
    unsigned size = sslReadSize(ssl);
    return sslRead(ssl, size);
}

Is there a buffering problem as in my old code?

\$\endgroup\$
3
\$\begingroup\$

Looks good. No buffering problems anymore. Endianness is handled correctly. Reading and writing size is duly factored out.

The only suggestion is not rely on the native unsigned type to represent the size. Notice that htonl takes uint32_t as a parameter. It is a strong hint to pass size as uint32_t as well. This guarantees that the size is 4 byte long no matter which platform it is running on. Don't forget to #include <cstdint>.

\$\endgroup\$
  • \$\begingroup\$ Do I have only replace unsigned to uint32_t? \$\endgroup\$ – Ufx Aug 25 '15 at 23:09
  • \$\begingroup\$ @Ufx Precisely. \$\endgroup\$ – vnp Aug 25 '15 at 23:09
  • \$\begingroup\$ Can I make palpably faster this code? \$\endgroup\$ – Ufx Aug 25 '15 at 23:53
1
\$\begingroup\$

I would change two things and one bug.

1: Checking for compatability is a compile time option.

if (sizeof(u_long) > 8)
    throw std::runtime_error("Read size error.");

// prefer to generate compile time errors when you can.

static_assert(sizeof(u_long) > 8, "Only support systems with the correct size");

2: Make your functions symmetric. If you have a write string then I would expect a read string. If you have a read vector I would expect a write vector.

3: Don't use at() unless you are validating user input. In this case it will throw an exception if the size of your string is 0. I would still expect your code to work correctly for zero sized strings.

 sslWrite(ssl, &data.at(0), data.size());   // throws for zero sized strings.

 sslWrite(ssl, &data[0], data.size());

Is there a buffering problem as in my old code?

I bet the SSL layer is already buffering this. There is no point in doing it twice. But its always worth reading the documentation to check.

\$\endgroup\$

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.