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

Question

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);

    //Convert the hex string back to binary
    Utility::string_to_binary(hex, data, size);

    //Cleanup
    delete[] data;
}

The error occurs on delete[] data.

Well, you are not only breaking the strict aliasing rule, but also alignment rules. You can't just take a memory address and interpret it as a pointer to a different type. — Šimon Tóth
– Šimon Tóth, Commented Sep 9, 2011 at 15:18
Welcome to Stack Overflow! The problem may be in the code that you are not showing us. Please provide a small, complete program that demonstrates the problem. For information about why that is important, and how to do it, see sscce.org. — Robᵩ
– Robᵩ, Commented Sep 9, 2011 at 15:19
Thanks. From that example, it is clear that @Kerrek and I both provide the correct answer. Please accept the one that is most useful to you by clicking the appropriate checkmark. — Robᵩ
– Robᵩ, Commented Sep 9, 2011 at 15:30
P.s. If you code on Linux, I recommend making valgrind part of your daily routine. Running the above test program under valgrind detects this particular error. — Robᵩ
– Robᵩ, Commented Sep 9, 2011 at 16:27

Robᵩ · Accepted Answer · 2011-09-09 15:42:54Z

3

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.

EDIT

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;
}

edited Sep 9, 2011 at 15:42

answered Sep 9, 2011 at 15:24

Robᵩ

170k20 gold badges251 silver badges323 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

ctrlc-root Over a year ago

Yeah, I figured, but I needed another pair of eyes. Thanks.

Kerrek SB · Accepted Answer · 2011-09-09 15:48:50Z

3

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

edited Sep 9, 2011 at 15:48

answered Sep 9, 2011 at 15:22

Kerrek SB

480k96 gold badges904 silver badges1.1k bronze badges

5 Comments

Robᵩ Over a year ago

As a long-time C programmer, I take offense that you think that looks like C. :)

Kerrek SB Over a year ago

@Rob: MY sincerest apologies! It was truly a question of who to insult least :-)

ctrlc-root Over a year ago

How would you write it in "C++"?

Kerrek SB Over a year ago

No pointers, and use std::string to your advantage. For the core conversion I'd probably use strtoul() on the string-to-binary side, and lookup in a static array on the binary-to-array side. Anyway, you seem to have found a sufficient answer, so let's just leave it at that.

Robᵩ Over a year ago

Nit: I think he is only trying to write one byte, not two. destination[b] advances by one byte per loop iteration.

dimitri · Accepted Answer · 2011-09-09 16:08:33Z

2

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();
}

answered Sep 9, 2011 at 16:08

dimitri

9748 silver badges11 bronze badges

1 Comment

ctrlc-root Over a year ago

Yeah, I got that from Kerrek SB above. Thanks for giving me an example, though. I'll definitely consider rewriting it in this fashion once time permits. (Thankfully, the class is in a shared library, so updating the code will be easy)

Collectives™ on Stack Overflow

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

3 Answers 3

1 Comment

5 Comments

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

1 Comment

5 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related