CODEWITHSUNDEEP
c++memory-managementheap-corruption
lebron2323
lebron2323

Reputation: 1040

Why i have HEAP CORUPTION in this simple code?

I have function which converts Hex String to Byte Array, 

BYTE* HexStrToByteArray(std::wstring hex_str)
{
    int len = hex_str.size()*0.5f;
    BYTE *bytearray = new BYTE[len];
    for(int i = 0; i < len; i++) 
    {
        swscanf(hex_str.c_str() + 2*i, L"%02x", &bytearray[i]);
    }
    return bytearray;
}

In code i use it like this

BYTE *byte_array = HexStrToByteArray(hex_str);

for now function work fine, but when i try release memory, allocated in function

delete [] byte_array;//HEAP CORUPTION ERROR

I have HEAP CORUPTION error... What i do wrong?

Upvotes: 2

Views: 159

Answers (3)

Roddy
Roddy

Reputation: 68054

You're just using the wrong format specifier for your swscanf, so writing four bytes for every hex pair. Use the hh argument type modifier. http://en.cppreference.com/w/c/io/fscanf

Try this instead.

swscanf(hex_str.c_str() + 2*i, L"%02hhx", &bytearray[i]);

As others have said, this is a pretty poor way to implement this function, anyway.

Upvotes: 1

user529758
user529758

Reputation:

Running valgrind on your code reveals the problem pretty quickly:

Invalid write of size 4
==34783==    by 0x21B719: swscanf
Address 0x100004000 is 0 bytes inside a block of size 3 alloc'd

I've got this error when I ran your code on a hex string of length 6:

BYTE *ba = HexStrToByteArray(L"123456");

Which, of course, is supposed to produce 3 bytes. However, the %02x conversion specifier makes swscanf() expect a pointer to unsigned int (which happens to be 4 bytes long on your implementation), whereas you are passing it a BYTE *, which is presumably a pointer to unsigned char instead.

Instead of trying to mess around with scanf(), which is horrible, use strtoul(). Also, use std::size_t for sizes, and please don't pollute integer operations with floating-point numbers. Furthermore, if your hexadecimal string is of an odd number of characters, you will need to allocate one extra byte for it. In addition, I suggest you pass the input string by const reference to avoid unnecessary copies.

BYTE *HexStrToByteArray(const std::wstring &hex_str)
{
    std::size_t len = (hex_str.size() + 1) / 2;
    char buf[3] = { 0 };
    BYTE *bytearray = new BYTE[len];

    for (std::size_t i = 0; i < len; i++) {
        buf[0] = hex_str[2 * i + 0];
        buf[1] = hex_str[2 * i + 1];
        bytearray[i] = std::strtoul(buf, NULL, 16);
    }

    return bytearray;
}

Upvotes: 3

Michael Burr
Michael Burr

Reputation: 340366

The "%02x" format spec expects an unsigned int value to write into, so instead of writing a single byte in each loop iteration it's writing 4 bytes.

So you're writing past the end of the allocated bytearray.

Upvotes: 3

Related Questions