I cannot magically debug code. But I can point out sources of bugs.
"VALUE_NAME_MAX_SIZE" -- there is no valid reason to use a fixed size array. Use a std::vector<TCHAR> if you need a buffer of TCHAR. Yes, this adds an indirection. But fixed sized buffers are the source of what is possibly the most common source of bugs in the entire world, and you aren't a good enough programmer to write bug-free code (hint: nobody is). Why in the world would you expose yourself to the most common source of bugs in the entire world needlessly?!
As an example of why what you did (a fixed size buffer) was stupid:
you just changed the length of the variable that describes how long valueName is. Without changing the size of valueName.
Then, you pass it to some other API, with the extended length.
Code: Select all
data = (LPTSTR) malloc(dataSize + sizeof(TCHAR));
Don't use malloc. Use a std::vector<TCHAR> to store that data, and resize it as needed. (or a CString if you prefer, you can get raw buffer access).
Code: Select all
result = RegEnumValue(hKey, (DWORD) i, valueName, &valueNameLength, NULL, &dataType, (LPBYTE) data, &dataSize);
Don't store the size and the array pointer in seperate variables you manipulate independently. That is a bug waiting to happen. By storing it in a vector, the size() of the vector is tied to the buffer itself. (To get a pointer to the contents of the vector, &vecname.front() works well). If you need to null terminate it, vecname.push_back(_T('0')); does it and manages memory as well.
Similar techniques for a CString exist.
Repeat after me: "If I call free or delete outside of a class that represents the ownership of data, I have made a mistake. It may not be a bug this time, or today, but it will be a bug tomorrow, or next time."
use reinterpret_cast<LPBYTE>(data) instead of (LPBYTE)data. For a few reasons.
First, never use C-style casts. If you don't know why never to use C-style casts, that is just evidence that you don't know enough not to use them. If you do know why to never use C-style casts, why are you doing it? Is it because it makes you feel dirty?
Second, because C++ explicit casts are explicit. They do what they claim to do. C-style casts will often succeed when they really shouldn't, and will do so silently. Sometimes they succeed in your test case, and then fail in some future corner case, because you where doing a reinterpret cast implicitly when you should be doing a static cast, and something really minor changed which makes the reinterpret cast fail.
As a simple rule, use static_cast if it works, and if it doesn't try reinterpret cast after knowing that the meaning is to reinterpret the exact bits as another type (and you should know the bit representation of everything involved in the casts down to the standard ideally before doing this...) And don't be afraid of marshalling data between types if you have to -- safety is better than speed, because speed is easy to add, while safety is hard to add.
Anyhow, I think I stumbled upon the fix to your bug while I looked at flaws in your code. Hope that helps!