The buffer size parameter to GetFileVersionInfo is the size of your buffer, no really


The GetFileVersionInfo function takes a pointer to a buffer (lpData) and a size (dwLen), and that size is the size of the buffer, in bytes.

No really, that's what it is.

The application compatibility folks found one popular game which wasn't quite sure what that dwLen parameter meant. The programmers must have thought it meant "The size of the version resources you want to load" and called it like this (paraphrased):

void CheckFileVersion(LPCTSTR pszFile)
{
 BYTE buffer[1024];
 DWORD dwHandle;
 DWORD dwLen = GetFileVersionInfoSize(pszFile, &dwHandle);
 if (GetFileVersionInfo(pszFile, dwHandle, dwLen, buffer)) {
  ...
 }
}

"Gosh, the GetFileVersionInfo function wants to know how big the version info is, so we need to call GetFileVersionInfoSize to find out!" they must have thought.

This code worked great... for a while. It was checking the file version of the video driver. (My guess is that they were trying to detect specific video drivers so they could work around bugs in them or take advantage of driver-specific features.) But if you had a video driver whose version resource needed more than 1024 bytes of space, the program crashed with stack corruption.

I don't know whether the Windows Vista application compatibility folks decided that it was worth fixing this program's bug, since it occurred even on Windows XP. If so decided, the fix would have been fairly straightforward. Once the program was detected, we would just have had to take the value the program passed as dwLen and modify it according to the simple formula

dwLen = min(dwLen, 1024);

before doing the real work of loading the version information.

For those playing along at home, by the way, the correct code would go something like this:

void CheckFileVersion(LPCTSTR pszFile)
{
 DWORD dwHandle;
 DWORD dwLen = GetFileVersionInfoSize(pszFile, &dwHandle);
 if (dwLen) {
  BYTE *pBuffer = (BYTE*)malloc(dwLen);
  if (pBuffer) {
   if (GetFileVersionInfo(pszFile, dwHandle, dwLen, pBuffer)) {
    ...
   }
   free(pBuffer);
  }
 }
}

(Use your favorite memory allocation technique instead of malloc and free.)

Comments (30)
  1. Adam says:

    Is it usual for the ‘dwLen’ parameter to come before ‘lpData’? I thought most buffer/size parameter pairs had the buffer parameter first.

  2. John says:

    I have always wondered how the whole application compatibility thing works.  Is there some documentation I can read about this somewhere?  I wouldn’t have imagined a developer of a popular (presumably advanced) game just handing the source code over to Microsoft to check for bugs or whatever.

  3. nativecpp says:

    This is not actually related to GetFileVersionInfo but is somewhat related to buffer size. I am always confused (i.e. need to read MSDN) about MAX_PATH.

    In GetTempFileName, the size should be MAX_PATH+1.

    In GetWindowsDirectory, the size should be MAX_PATH

    In GetModuleFileName, I am not sure since we can have long path (i.e. "\?")

  4. mikeb says:

    > I wouldn’t have imagined a developer of a popular (presumably advanced) game just handing the source code

    This kind of bug doesn’t need the original source – but it does take work looking at the disassembled code (work that by all rights Microsoft probably shouldn’t have to expend, but that’s another drifted thread of conversation…).  Strictly speaking, you can debug any code without source – it’s just usually more difficult.

    What you do is notice that GetFileVersionInfo() causes a crash, then when you look in the disassembled code, you see that the result from    GetFileVersionInfoSize() is being passed as the buffer size for a buffer that’s on the stack.

    Since the stack-based buffer is allocated before the GetFileVersionInfoSize() call (by manipulating the stack pointer), you can deduce that the length being passed to GetFileVersionInfo() has no relationship to the actual size of the buffer.

  5. Pierre B. says:

    Isn’t it much more likely that the programmer simply thought that 1024 bytes would always be quite enough?

    Since games programmers are known to cut even more corners than others, that’s a much more probable cause than confusion about the meaning of the parameter.

    I’ve seen so much code with hard-coded MAX_PATH buffers like that when manipulating files…

    [In that case, don’t ask for the size in the first place since you’re just going to ignore it. Just call GetFileVersionInfo with a buffer size of 1024. -Raymond]
  6. JeffCurless says:

    And then windows would have become more bloated and crappy….

    I mean really, I wish I were a crappy coder, I would get a lot more done and then you guys could just fix it all for me!

    And I’m a windows guy, I hate other OSes…

  7. Arno Schoedl says:

    I know I am nit-picking, but quite a few of these API functions (GetWindowText and RegistryQueryString functions are others), access multi-threaded resources. Usually, you first call a …Length, then allocate the buffer, then call … Noone guarantees though that the buffer size you queried first is the buffer size that is still required when the data is received. Strictly, you need a loop, checking whether your buffer was big enough and trying again in case it was not.

  8. John says:

    I know it is possible to debug a relatively simple problem like that without source code (in fact I have done it), but I just cannot imagine Microsoft (or any other company) stepping through and analyzing hundreds of lines worth of assembly code for hundreds of products written by hundreds of developers just to fix stupid mistakes like this.

  9. DriverDude says:

    "…I just cannot imagine Microsoft (or any other company) stepping through and analyzing hundreds of lines worth of assembly code for hundreds of products written by hundreds of developers just to fix stupid mistakes like this."

    Obviously MS thinks the cost of fixing other vendors’ bugs is less than the cost of bad press saying "the latest windows breaks my game!"

    That’s good for business. But then there’s little incentive for the 3rd party to learn and fix THEIR mistakes.

    I’ve met sys admins who are so jaded about Windows they cannot understand why it cannot be MS fault when a service pack breaks an app. Until I explain those dumb stupid mistakes the app makes, like overwriting heap pointers, etc.

    Read Nvidia graphic driver release notes sometime. There’s often a half-dozen errata of the form "Game XYZ shows tearing when 4xAA is enabled. The game is using ABC function incorrectly. This is not a Nvidia bug."

    Ever wonder how many people Nvidia has in their compatibility labs, playing games all day?

    Microsoft Press should take Raymond’s Win32 postings and publish a book, "1001 Win32 Bugs to Avoid"  :=)

  10. Sven Groot says:

    "I’ve seen so much code with hard-coded MAX_PATH buffers like that when manipulating files…"

    Considering that Windows Explorer itself can’t handle paths longer than that, I don’t think that’s really a problem.

  11. jon says:

    By the way Raymond, there’s a bug in GetFileVersionInfoSize (at least under Vista, not sure about previous versions) – if you call it on a file that is > 4 gig in size it leaves a lock open on the file.

  12. John Hensley says:

    "Considering that Windows Explorer itself can’t handle paths longer than that, I don’t think that’s really a problem."

    I would bet that explorer doesn’t handle it because there’s no benefit to displaying longer paths to the user. A file path that is not displayed to the user is a different matter.

    Lots of developers will use MAX_PATH to declare the length of any string whatsoever. 260 is the new 80.

  13. Andre says:

    "That’s good for business. But then there’s little incentive for the 3rd party to learn and fix THEIR mistakes."

    Every application has bugs, some more, some less. Often bugs occurs years after a release because an API has been misused but worked in the past. The vendor may already out of business then, or in most cases has no interest in releasing a bug fix. I bet most software vendors couldn’t even release a bugfix 5 years after the last one. In that case it might make sense for the OS to fix the problem. Microsofts own products are no exception here.

    But I’d say the initial problem here is that the buffer and size parameter is swapped. It looks like the size parameter belongs to the handle and not to the buffer. The Win32 API is inconsistent here.

  14. Anony Moose says:

    Nah, the devs obviously knew which param was the size – they just didn’t have a clue what to actually put in it, and didn’t even bother to check that the buffer was large enough. The Win32 API may be inconsistent here, but that’s not the cause of the problem.

  15. dave says:

    This is a variation on the same theme, that I’ve seen from time to time:

      strncpy(dest, src, strlen(src)+1);

    Well, I suppose they at least understand that they *want* to prevent buffer overflow.

  16. Igor says:

    "260 is the new 80."

    Surely you mean 81? ;)

    By the way, those who speak about calling an API in a loop to get the right size don’t have a clue — most APIs can be called with 0 (zero), in that case required buffer length is returned. God forbid if we really had to call something several times in a loop for each of 50,000 files.

  17. Gabe says:

    Igor, the problem with requesting the buffer length before calling the API is that the size of the data may have changed between the two calls.

    For example, let’s say that I’m trying to read a 100-byte registry value with RegGetValue(). I call it with a value of 0 for pcbData, and the function returns ERROR_MORE_DATA and tells me that I need 100 bytes. I then allocate the 100 byte buffer.

    Assume that now some other process changes the registry value to 150 bytes. When I call RegGetValue again with my 100-byte buffer, I will get ERROR_MORE_DATA again and pcbData will now have 150. I have to call RegGetValue again with a reallocated 150-byte buffer, and hope that somebody didn’t make it bigger again.

    This isn’t such a big problem with the registry because I know that a value can’t be more than 64k so I can just always have a buffer of maximum possible size. In Vista I could use a transaction to make sure nobody modifies the key while I’m looking at it. However, this is just an example of a situation where you have to call the API in a loop if you want to reliably get the necessary buffer size.

  18. Centaur says:

    @Igor

    most APIs can be called with 0 (zero), in that case

    required buffer length is returned.

    Yes, you call the function with 0, it tells you the required buffer size. Then a thread and/or process switch happens, another application changes a value in the registry. Then you get control back, allocate the memory using the size that was sufficient 20 milliseconds ago, call the function again with the new buffer, and end up with a truncated string and an ERROR_MORE_DATA status code. “Impossible! It told me I only need 42 bytes, I gave it 42 bytes and it says it’s not enough!?” Oh well, Windows API only gives a Read Committed transaction isolation level, not Repeatable Read.

  19. DriverDude says:

    Andre, stop making excuses.

    MSDN: "dwLen: Specifies the size, in bytes, of the buffer pointed to by the lpData parameter."

    In this case, there is no freaking way to be confused about what the size param is meant for. It does not make any sense to ask an API for the "size of a handle"  There is no way to mixup the handle and size – one is a typedefed pointer, the other is an unsigned int. The compiler will tell you.

    There are many other examples of "get the sizeof data" and then "get the data"  Many APIs have such mechanisms, for many things – not just Win32.

    MS has its share of bugs and poor docs, but this is the kind of mistake a first-year student makes.

    If the vendor has no interest in fixing *their own bugs*, or if they cannot, then why is it Microsoft’s job to do it for them? (Other than being good for MS. But it doesn’t seem to improve overall software quality!)

    Maybe Microsoft should hand-hold developers too?

  20. Lisianthus says:

    If Raymond is referring to the game I think he is, I’m afraid I was involved in this particular disaster. :(

    The truth is, we found out about this bug soon after ship, and we were very embarrassed that a fixed size buffer had been hard-coded. I’m pretty sure that the buffer was fixed with proper dynamic allocation in the very first patch, but by that time it was too late, given that many people never bother to update and we didn’t have a way to push a patch.

    So, sorry for the lame code, and that’s one bug we’re not eager to repeat.

  21. Igor says:

    Gabe, thanks for the reminder of that particular (ugly) case.

    Guess I never needed to read a registry key which someone is modifying at the same time. I believed that the registry has support for atomic operations. Obviously I was wrong.

    I still don’t understand why system cannot allocate the proper buffer, copy the data and return the pointer and the length in one go. True it would require RegFreeData() API but you wouldn’t have to call the same API dozen times to get one value.

    So that is the reason why people want to know absolute limits (Raymond posted recently about it) and why some programmers just adore allocating static buffers (like in this post only this one wasn’t big enough).

    Furthermore, will the value you get in the end be the one you expected to get? Or should your application fail when you realize that value has changed? What about nasty applications which constantly jerk off through the registry (pretty much anything that sits in taskbar notification area)? How much do they affect the system performance? Should Microsoft introduce some limit of registy accesses per second per process?

    Meditate upon that, I will.

  22. Norman Diamond says:

    Friday, March 30, 2007 1:27 AM by Igor

    By the way, those who speak about calling an

    API in a loop to get the right size don’t

    have a clue — most APIs can be called with 0

    (zero), in that case required buffer length is

    returned. God forbid if we really had to call

    something several times in a loop for each of

    50,000 files.

    You’re the one without a clue.  Read the MSDN page on GetModuleFileName and see what it returns.  God forbid you to code what you have to code in order to get correct results, eh?

    Don’t read the MSDN page on LoadString though.  Don’t read the MFC LoadString code that calls the Win32 LoadString API in a loop in order to figure out whether it got the entire string or not.  You’re obviously happier pretending that it doesn’t exist.  (Those of us who have clues do wish that it didn’t have to exist.)

  23. Mark Ingram says:

    I really would hope that seasoned game programmers would not be making "newbie" mistakes like that. I hope they put it down to a placement student ;)

  24. Norman Diamond says:

    > Just call GetFileVersionInfo with a buffer

    > size of 1024.

    You’re right.  The code in the article tricked me into being suspicious, so I looked it up.

    *  DWORD dwHandle,

    *  dwHandle

    *    This parameter is ignored.

    OK, the wording’s there and it’s correct.  But still, with or without a time machine, the thing isn’t

      HANDLE hSomethingOrOther

    which is always set to NULL and then ignored, and it isn’t

      HANDLE hIgnored

    which would make it more obvious to even a quick scanner.  Of course the code in the article has its own unacceptable bug, but we can understand a bit why the original coder was confused.

    Thursday, March 29, 2007 12:40 PM by nativecpp

    > In GetTempFileName, the size should be MAX_PATH+1.

    It doesn’t hurt to do that (wastes an amount of memory equal to one TCHAR’s size) but MSDN says MAX_PATH is enough.  (On the other hand, this MSDN page isn’t quite sure if it understands TCHARs, so I’m not quite sure if we should rely on its understanding of MAX_PATH.  Maybe we should use MAX_PATH+1.)

    > In GetWindowsDirectory, the size should be MAX_PATH

    Agreed.

    > In GetModuleFileName, I am not sure since we

    > can have long path (i.e. "\?")

    There is no apparent limit.  Whatever file system the file was stored in, the file system might set a limit, but the GetModuleFileName has no apparent limit here.  I think you have to put your call to GetModuleFileName in a loop, growing the buffer each time until the result length is shorter than the buffer length.

    Thursday, March 29, 2007 3:29 PM by Sven Groot

    >> "I’ve seen so much code with hard-coded

    >> MAX_PATH buffers like that when manipulating

    >> files…"

    >

    > Considering that Windows Explorer itself can’t

    > handle paths longer than that, I don’t think

    > that’s really a problem.

    Wrong, very wrong.  Here are some reasons, in the wrong order:

    (1)  Windows Explorer itself has problems with some paths shorter than that too.

    (2)  Paths can exist which can be longer than Windows Explorer can handle.  It’s pretty trivial to crash telephones which were made with Windows CE prior to version 5, which doesn’t help customers much.

    (3)  It’s pretty trivial for malware to write pathnames longer than that, and it’s pretty wise for anti-malware to find them and deal with them.

  25. Tim Smith says:

    Seasoned game programmers might be very good at creating games but poor when working with Win32.  When killing yourself to get those extra three frames per second, you tend to not care if your version info call is correct.

    Sad but true.

  26. Mak says:

    What would interest me is if all these compatibility hacks are dynamically loaded at the start of an application, or if they are statically integrated into the Windows APIs and therefore contributing to the alleged Windows "bloat"?

    Does a compatibility fix overhead affect only the intended process or all running processes?

    Are the compatibility fixes documented somewhere, or will API competitors like Wine or ReactOS always lack this features?

  27. Igor says:

    Norman, tsk, tsk, tsk…

    I said most APIs (I admit I should have said some instead), but I surely didn’t say all of them and I also wish that this mess didn’t have to exist. If I hate something in programming that is reallocating memory.

  28. Messiant R says:

    The more I read about the experiences from the application compatibility labs/people, the more it makes me want to work alongside them

    Keep those stories coming

  29. Sohail says:

    Is it just me or would all these problems go away if the Windows API had a FreeThisTypeOfPointer(Type*) function?

  30. Chuck Chen says:

    Mak,

    The compatibility *hacks* are dynamically loaded at the start of an application.  And they are only loaded for the intended processes, not for every running process.

Comments are closed.

Skip to main content