C++ Converting binary data to a hex string and back

I have a matching pair of static functions in a utility class that I use to convert between binary data (unsigned characters) and it's string representation (a-f and 0-9). They seemed to work correctly but recently I tried to compile my code under Visual C++ (2010 Express) and to my dismay, they cause nothing but heap corruption errors. What am I doing wrong?

void Utility::string_to_binary(const std::string source, unsigned char* destination, unsigned int length)
{
    unsigned int effective_length = min(length, (unsigned int) source.length() / 2);
    for(unsigned int b = 0; b < effective_length; b++)
    {
        sscanf(source.data() + (b * 2), "%02x", (unsigned int*) &destination[b]);
    }
}

void Utility::binary_to_string(const unsigned char* source, unsigned int length, std::string& destination)
{
    destination.clear();
    for(unsigned int i = 0; i < length; i++)
    {
        char digit[3];
        sprintf(digit, "%02x", source[i]);
        destination.append(digit);
    }
}

Edit: Here's a complete program that illustrates the problem.

#include <iostream>
#include <hdcs/Utility.h>

using namespace std;

int main(int argc, char* argv[])
{
    //Generate some data
    unsigned int size = 1024;
    unsigned char* data = new unsigned char[size];

    //Convert it to it's string representation
    string hex;
    Utility::binary_to_string(data, size, hex);

    //Output it to the screen
    cout << hex << endl;

    //Clear the data buffer
    memset(data, 0, sizeof(unsigned char) * size);

    /开发者_JAVA技巧/Convert the hex string back to binary
    Utility::string_to_binary(hex, data, size);

    //Cleanup
    delete[] data;
}

The error occurs on delete[] data.

In this code,

for(unsigned int b = 0; b < effective_length; b++)
{
    sscanf(source.data() + (b * 2), "%02x", (unsigned int*) &destination[b]);
}

you seem to be writing an unsigned int at locations destination, destination+1, destination+2, &c. As you approach the final bytes of your destination buffer, you will write beyond its limit.

For the sake of example, let us assume that destination is a four-byte buffer, and that sizeof (unsigned int) is 4 in your environment. Then each sscanf is writing four bytes.

The first iteration writes bytes 0, 1, 2, 3

The second iteratino writes bytes 1, 2, 3, 4

The third iteration writes bytes 2, 3, 4, 5

The final iteration writes bytes 3, 4, 5, 6

Since the buffer was only four bytes to start with, you have written beyond the end of your buffer. Boom.

The minimum change required to avoid this particular bug follows:

for(unsigned int b = 0; b < effective_length; b++)
{
    unsigned int ui;
    sscanf(source.data() + (b * 2), "%02x", &ui);
    destination[b] = ui;
}

Your sscanf will write an unsigned int into the memory location you give it. Typically, an unsigned int is 4 or 8 bytes long, while you only intend to provide 1 byte. So at the end you're running flat-out over the end of your dynamic array.

By the way, your code is very far removed from modern, idiomatic C++ - it's essentially just a glorified C mess. I strongly suggest rewriting it in the spirit of C++.

I would rewrite the code to actually use C++ facilities (haven't tested it really, just an idea):

std::vector<unsigned char> string_to_binary(const std::string& source)
{
    static int nibbles[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 0, 0, 0, 0, 0, 0, 10, 11, 12, 13, 14, 15 };
    std::vector<unsigned char> retval;
    for (std::string::const_iterator it = source.begin(); it < source.end(); it += 2) {
        unsigned char v = 0;
        if (std::isxdigit(*it))
            v = nibbles[std::toupper(*it) - '0'] << 4;
        if (it + 1 < source.end() && std::isxdigit(*(it + 1)))
            v += nibbles[std::toupper(*(it + 1)) - '0'];
        retval.push_back(v);
    }
    return retval;
}

std::string binary_to_string(const std::vector<unsigned char>& source)
{
    static char syms[] = "0123456789ABCDEF";
    std::stringstream ss;
    for (std::vector<unsigned char>::const_iterator it = source.begin(); it != source.end(); it++)
        ss << syms[((*it >> 4) & 0xf)] << syms[*it & 0xf];

    return ss.str();
}