current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

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?

share|improve this question
up vote 2 down vote accepted

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>.

share|improve this answer
    
Do I have only replace unsigned to uint32_t? – Ufx Aug 25 '15 at 23:09
    
@Ufx Precisely. – vnp Aug 25 '15 at 23:09
    
Can I make palpably faster this code? – Ufx Aug 25 '15 at 23:53
up vote 1 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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