SOLUTION: Spotting Code Defects #2 (Accessing Registry Values)

  • Article

This defect seems to have led a few more people astray then the last.  While no one posted publicly several people emailed me solutions.  Thanks to all who contributed!

 

So let’s start with the hints:

 

Hint #1: It is not only important to test return values, but also to make sure you understand how to get extended error information.

 

Several people saw this up front and the hint helped a few more.  The Reg* functions do not call SetLastError and therefore GetLastError will return bogus information.  The return value from the Reg* function is the error code.  The Windows API is not always consistent.  Some HANDLES are invalid if they are NULL, others if they are INVALID_HANDLE_VALUE.  Some functions call SetLastError, others don’t.  The bottom line is to always double check.

 

Hint #2: What is the potential range of values for a LONG?

 

Only 2 people noticed this in the first go-round.  Consider the line of code in question:

 

LONG cbLength = (LONG)cchLengthIn * sizeof(TCHAR);

 

If we are in a UNICODE build (or anytime sizeof(TCHAR) > 1) then there is a chance we’ll have an overflow and wrap-around.  So it’s possible that cbLength will be a negative number.  Why is that a problem?  Well, consider what would happen if cchLengthIn were 1073741824 or greater?  In a UNICODE build it would become a negative number.

 

Now let’s think extra hard for a second … RegQueryValue has the following signature:

 

LONG RegQueryValue(

  HKEY hKey,

  LPCTSTR lpSubKey,

  LPTSTR lpValue,

  PLONG lpcbValue

);

 

But RegQueryValueEx has the following signature

 

LONG RegQueryValueEx(

  HKEY hKey,

  LPCTSTR lpValueName,

  LPDWORD lpReserved,

  LPDWORD lpType,

  LPBYTE lpData,

  LPDWORD lpcbData

);

 

Anything catch your eye?

 

Look at lpcbValue in RegQueryValue … it’s a PLONG.  But lpcbData, the comparable parameter, is a LPDWORD.

 

Why do we care?

 

Well – let’s play a little game…  It’s one of my favorites.  It’s called “Crash the program.”  You may have heard of it.  Heck, you may have written your own version of it in the past.

 

Consider this program:

 

#include <tchar.h>

#include <windows.h>

int _tmain()

{

      TCHAR lpszVersion[1];

      LONG cbLength = -1;

      RegQueryValue(HKEY_CURRENT_USER, _T("Software\\MagicApp"), lpszVersion, &cbLength);

}

Compile it as UNICODE.  Also – assume that the cbLength is bogus because of a bug in the program (like an integer overflow), it’s hardcoded to make it clear what’s going on.  Also assume that lpszVersion is not [1], I did that to make it obvious too … and assume that the key we’re reading from has lots and lots and lots of data (well … more then 1 character anyway :).  Use your imagination – it’s not hard.

 

Now we know that RegQueryValueW is actually what we’re calling.  And we know that RegQueryValueW is in advapi32.dll.  So let’s open that DLL in depends.exe (this is a PSDK tool - but this page also allows you to download it without the entire PSDK.  If you have Visual Studio installed - you already have this.  I put it in my Send To menu so I can right-click DLL's and open then with Depends.exe).

 

Now we can see that (on my machine, anyway) RegQueryValueW has an entry point of 0x00004367 relative to the load point of the module.  Since it is possible that the DLL might be relocated let’s only worry about the offset for now.

 

Now start debugging using F10 (don’t F5 it … it will just crash randomly.  We want to trace the execution).

 

When you get to the first line hit CTRL+ALT+U.  This will open up the module window.  Advapi32 is loaded at 0x 77DA0000-0x 77E30000.  So let’s set a breakpoint on 0x77DA4367 (0x77DA000+0x4367).  Now hit F5.  If you didn't hit the break point try starting over with a new debugging session and hitting CTRL+ALT+D to view your disassembly and trace through what is happening.

 

Before we go any further … you do have symbols loaded, right?  Check this URL for more info:

 

https://support.microsoft.com/default.aspx?scid=kb;en-us;319037

 

Still don't have symbols?  Please - I'll wait.  This is worth the effort.  Believe me.

 

Alright … let’s keep going.

 

Now, for me … at 0x77DA443B (that is 0x77DA4367+D4) there is a call opcode to 0x77DA2FAA.  Now that was in that advapi32.dll range we saw (and since we are using symbols we know) – it’s calling RegQueryValueExW.  Oh man … that LPLONG just became an LPDWORD.  We just took our negative value and made it positive.  HUGELY positive.

 

By this time you know that when you hit F5 again – you’re going to crash (if the key exists and contains too much data).

 

I talk too much.  Let’s go on to the next hint.

 

Hint #3: Is the caller given enough information on error?

 

No, the caller is not.  When RegQueryValue[Ex] fails it gives you back the length you would need.  We need to pass that back as well.  If we don’t then how does the caller stand a chance of getting it right the second time (or the third, or forth … who knows how many times you might have to guess)!  The function needs to define some interface for passing back this information to the caller.  Perhaps another parameter, perhaps a ERROR_MORE_DATA style return code, something.  Doesn’t really matter to me except that it has to do it.

 

Hint #4: https://dictionary.reference.com/search?q=deprecated

RegQueryValue is deprecated.  Use RegQueryValueEx instead.  If we had we would have started with a DWORD and hopefully done it right the first time.  Also we avoid, as we just saw, an unnecessary level on indirection.  Take the extra 4 seconds and use the right function.

 

Summary (about time, too)

 

So we have a serious integer overflow problem, two moderate design problems and we’re not handling failure correctly (also serious).  Considering we’re talking about a function with only a handful of statements this is not good  – but we’re better off now then we were 30 minutes ago.