RegEnumValue in Windows 7

A place to discuss the implementation and style of computer programs.

Moderators: phlip, Moderators General, Prelates

User avatar
TheAmazingRando
Posts: 2308
Joined: Thu Jan 03, 2008 9:58 am UTC
Location: San Diego, CA

RegEnumValue in Windows 7

Postby TheAmazingRando » Mon Jun 04, 2012 11:26 pm UTC

I'm writing a program that has to interact quite a bit with the windows registry. While it works just fine on the computer I'm developing on, I've encountered a few mysterious crashes while porting it. The crashes are inconsistent, which leads me to believe it's probably a memory problem, but I can't seem to find it. The poorly documented behaviors of the Windows API functions I'm working around don't make it easier.

If anyone can point me in the right direction I would be very thankful.

Basically, given a particular registry key, I'm trying to cycle through the data of each value so I can operate on it. I logged it and determined that it makes it through several cycles before crashing near the second query.

Code: Select all

//numValues is the number of values in the current key
//all values are type REG_SZ
for(int i = 0; i < (signed int) numValues; ++i) {
    DWORD valueNameLength = VALUE_NAME_MAX_SIZE;
    LPTSTR data = NULL;
    DWORD dataType = 0;
    DWORD dataSize = 0;
    TCHAR valueName[VALUE_NAME_MAX_SIZE];

    result = RegEnumValue(hKey, (DWORD) i, valueName, &valueNameLength, NULL, NULL, NULL, &dataSize);

    //need to increment value name length to account for null termination
    ++valueNameLength

    //string is not guaranteed to be null terminated, add null termination
    data = (LPTSTR) malloc(dataSize + sizeof(TCHAR));
    data[dataSize / sizeof(TCHAR)] = '\0';

    result = RegEnumValue(hKey, (DWORD) i, valueName, &valueNameLength, NULL, &dataType, (LPBYTE) data, &dataSize);

    std::wstring dataString(data);

    *snip*
    free(data);
}

User avatar
Sc4Freak
Posts: 673
Joined: Thu Jul 12, 2007 4:50 am UTC
Location: Redmond, Washington

Re: RegEnumValue in Windows 7

Postby Sc4Freak » Tue Jun 05, 2012 6:34 am UTC

Have you tried running through a debugger? If not, that's your first step. If you're able to reproduce it and you can attach a debugger, then diagnosing the problem should be fairly trivial.

User avatar
TheAmazingRando
Posts: 2308
Joined: Thu Jan 03, 2008 9:58 am UTC
Location: San Diego, CA

Re: RegEnumValue in Windows 7

Postby TheAmazingRando » Tue Jun 05, 2012 3:17 pm UTC

I've debugged it pretty extensively on my machine, but I can't reproduce the error locally. I was hoping I wouldn't have to install a development environment on the computer it's failing on, it's a shared test computer and my coworkers like to keep it clean in order to test deployment, but I suppose it may come to that.

User avatar
Yakk
Poster with most posts but no title.
Posts: 11115
Joined: Sat Jan 27, 2007 7:27 pm UTC
Location: E pur si muove

Re: RegEnumValue in Windows 7

Postby Yakk » Tue Jun 05, 2012 3:42 pm UTC

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

Code: Select all

    free(data);

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

Code: Select all

 (LPBYTE) data

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!
One of the painful things about our time is that those who feel certainty are stupid, and those with any imagination and understanding are filled with doubt and indecision - BR

Last edited by JHVH on Fri Oct 23, 4004 BCE 6:17 pm, edited 6 times in total.

User avatar
TheAmazingRando
Posts: 2308
Joined: Thu Jan 03, 2008 9:58 am UTC
Location: San Diego, CA

Re: RegEnumValue in Windows 7

Postby TheAmazingRando » Tue Jun 05, 2012 6:36 pm UTC

Thanks for the help. I'll take a look at what you mentioned.

Yakk wrote:As an example of why what you did (a fixed size buffer) was stupid:
++valueNameLength
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.
Thanks to a strange feature of the API, valueNameLength is actually set by RegEnumValue to one less than the actual amount of characters it stores in valueName. Until I increment it, it isn't accurate, and a second call to RegEnumValue will fail because of insufficient space.

Anyway, I realize a fixed size buffer is a stupid idea. I switched to it while trying to fix it because it's the way it's done in the API documentation. I figured if someone familiar with the API had any suggestions, doing things as canonical as possible might help highlight where I'm going wrong.

User avatar
Yakk
Poster with most posts but no title.
Posts: 11115
Joined: Sat Jan 27, 2007 7:27 pm UTC
Location: E pur si muove

Re: RegEnumValue in Windows 7

Postby Yakk » Tue Jun 05, 2012 6:48 pm UTC

Sure. What happens if the string is of the max length -- I'm guessing it will NOT be null terminated? Then you just screwed yourself.

Incrementing it was a bad idea -- setting it back to the constant is a better idea. Don't treat quirks of the API as something reliable, not unless you absolutely have to.

Another problem:

Code: Select all

data[dataSize / sizeof(TCHAR)] = '\0';[/cod]
if dataSize isn't actually a multiple of TCHAR, you just set the second-to-last TCHAR to 0.  And then proceeded to presume the value was null terminated.
[code]
std::vector<TCHAR> data;
data.resize( (dataSize+sizeof(TCHAR)-1)/sizeof(TCHAR) );
Assert(data.size() * sizeof(data[0]) == dataSize );
Assert(!data.empty());
if (data.empty())
  data.resize(1);
result = RegEnumValue(hKey, (DWORD) i, valueName, &valueNameLength, NULL, &dataType, reinterpret_cast<LPBYTE>(&data.front()), dataSize);
Assert(data.size() * sizeof(data[0]) == dataSize );
data.push_back( _T('\0') ); // or TCHAR().

which uses a variable sized buffer, handles memory management, and asserts a few corner cases (such as the API saying "I don't need no stinking bytes").

If you are using TCHARs, you should be using std::basic_string<TCHAR> instead of wstring btw.
One of the painful things about our time is that those who feel certainty are stupid, and those with any imagination and understanding are filled with doubt and indecision - BR

Last edited by JHVH on Fri Oct 23, 4004 BCE 6:17 pm, edited 6 times in total.

User avatar
TheAmazingRando
Posts: 2308
Joined: Thu Jan 03, 2008 9:58 am UTC
Location: San Diego, CA

Re: RegEnumValue in Windows 7

Postby TheAmazingRando » Tue Jun 05, 2012 7:07 pm UTC

Switching to vectors did the trick. Thanks!

User avatar
Yakk
Poster with most posts but no title.
Posts: 11115
Joined: Sat Jan 27, 2007 7:27 pm UTC
Location: E pur si muove

Re: RegEnumValue in Windows 7

Postby Yakk » Tue Jun 05, 2012 7:50 pm UTC

... and why did it? If you where corrupting memory, you'd change from corrupting the stack to corrupting the heap. Heap corruption is less likely to cause an immediate crash than a stack corruption.

So if the only thing you did was change to vectors, you could easily be still be making the same mistakes, just not crashing immediately. Of course, you could easily have done other things in the change to vectors which would actually solve your problem. :)

Did you figure out why you where crashing?
One of the painful things about our time is that those who feel certainty are stupid, and those with any imagination and understanding are filled with doubt and indecision - BR

Last edited by JHVH on Fri Oct 23, 4004 BCE 6:17 pm, edited 6 times in total.

User avatar
TheAmazingRando
Posts: 2308
Joined: Thu Jan 03, 2008 9:58 am UTC
Location: San Diego, CA

Re: RegEnumValue in Windows 7

Postby TheAmazingRando » Wed Jun 06, 2012 12:56 am UTC

Looks like the null termination was overflowing the valueName buffer.
Which is to say, the vectors themselves didn't fix it, but rewriting it using vectors and your other suggestions did.


Return to “Coding”

Who is online

Users browsing this forum: No registered users and 9 guests