Reading a REG_BINARY corrupt the method's parameters

Question

I'm trying to read a REG_BINARY using the following code. value ends up with the correct string but I get an Acces violation error after the return statement. After some debugging, I found that the method's parameters gets overwritten with crap after the execution of RegQueryValueExW (for example, name starts with "test" and becomes "Î\0ÎÎÎôvv¦&..." after RegQueryValueExW).

string GetREG_BINARYValue(HKEY hkey, DWORD cbData, wstring name)
{
    DWORD* data = new DWORD[cbData]; 
    DWORD size = cbData;

    if( RegQueryValueExW(hkey,name.c_str(), NULL, NULL, reinterpret_cast<LPBYTE>(&data), &size)  != ERROR_SUCCESS)
    {
        RegCloseKey(hkey);
        return "Could not read REG_BINARY registry value";
    }    

    string value =  ByteArrayToString((BYTE*)&data, size);
    return value;
}

The function is called from this method:

string RegistryReader::ReadRegValue(HKEY root, string key, string name)
{
    HKEY hkey;
    DWORD type;
    DWORD cbData;
    string result;
    wstring wkey, wname;
    std::wstring_convert<std::codecvt_utf8_utf16<wchar_t>> converter;
    wkey=converter.from_bytes(key);
    wname=converter.from_bytes(name);

    if (RegOpenKeyExW(root, wkey.c_str(), 0, KEY_ACCESS_DEF64 , &hkey) != ERROR_SUCCESS)
    {
        if (RegOpenKeyExW(root, wkey.c_str(), 0,  KEY_ACCESS_DEF32, &hkey) != ERROR_SUCCESS)
        {
            return "Could not find registry key";      
        }  
    }

    if (RegQueryValueExW(hkey, wname.c_str(), NULL, &type, NULL, &cbData) != ERROR_SUCCESS)
    {
        RegCloseKey(hkey);
        return "Could not open registry value";
    }

    switch (type)
    {
    case REG_SZ:
        result=GetREG_SZValue(hkey, cbData, wname);
        break;
    case REG_DWORD:
        result=GetREG_DWORDValue(hkey, cbData, wname);
        break;
    case REG_BINARY:
        result=GetREG_BINARYValue(hkey, cbData, wname); //<--Here
        break;
    default:
        return "Invalid data type";
        break;
    }
    return result;
}

At first I though it was an array size problem but I do end up with the correct result, and cbData always contains the size I expect. I'm probably doing something wrong with RegQueryValueExW but I'm having a hard time finding explicit documentation. Is there something obviously wrong with this code?

Answer 1

There are many things wrong with your code. I listed lots of them in my comment to the question. The fundamental problem is that you must pass to RegQueryValueExW a pointer to an array of length size. But you do not.

Now, data is a pointer to an array of DWORD, of length size. So it contains four times as many bytes as you need. That in itself is not so bad. But instead of passing data, you pass &data. Which is the address of the pointer rather than the address of the array. You then invoke undefined behaviour when the API function writes over the pointer rather than writing to the buffer to which it points.

The fact that you had to cast should have been the first warning sign. Try not to cast. I repeat, try not to cast. When you cast you stop the compiler finding your mistakes. When the compiler tells you that you made a mistake, don't ignore it with a cast. Listen to it.

You need to allocate an array of BYTE, if we must use these all-caps Windows types:

BYTE* data = new BYTE[cbData]; 

Now you can pass data to the API function rather than &data. Obviously pass data to ByteArrayToString also. And do remember to delete the array when you are done with it.

As I said in my comment to the question, there are loads of other mistakes, and I don't really want to get into debugging and correcting all of them. I've hopefully given you a good steer in that comment and the rest is down to you.