Amusing bug in GetTempPath()

Yesterday I had a bug report from a client who had a user that was getting an exception report from their software which simply said “GetTempPath() - Failed to get temp path”. Now as error messages go this isn’t the best but it comes from some code that has been in my tools libraries for around 10 years and which has never failed before, it has no tests, we’re probably lucky that the message didn’t just read “TODO” as I’m pretty sure that it’s the first time that anyone has ever seen it apart from by reading my source code or running strings on an exe that includes my source code… Anyway…

The offending code was a wrapper around the operating system function `GetTempPath(). I always tend to wrap these kinds of things so that I can convert errors to exceptions (I did a good job of that here didn’t I) and so that I can convert types to the kind of types that I prefer to use. In this case I also dealt with the fact that you need to call the API without a buffer to determine how big a buffer it requires and then call it again with the correct sized buffer to obtain the path.

`GetTempPath() is a simple function and the documentation is pretty clear on how it should be used. My original wrapper was fairly crap.

_tstring GetTempPath()
{
   const DWORD spaceRequired = ::GetTempPath(0, 0);      // 1
  
   _tstring directory;
  
   directory.resize(spaceRequired - 1);     // 2
  
   const DWORD spaceUsed = 1 + ::GetTempPath(
      spaceRequired,
      const_cast<tchar*>(directory.c_str()));    // 3
  
   if (spaceRequired != spaceUsed)   // 4
   {
      throw CException(_T("GetTempPath()"),
         _T("Failed to get temp path"));
   }
  
   return directory;
}

First I called the function to determine the required size, note that the required “size is the size of the buffer in TCHARs” needed, which includes the terminating null character. Next I created a string that was the correct size (note that I’m removing 1 because the required size includes space for the null and I’m resizing the data area of the string and the null is added on for me. Thirdly I call the API again with my buffer. I then check that the second call uses all of the space that I allocated in the first call and fail if it doesn’t.

There’s quite a lot wrong with this code but mostly it will work… The check at stage 4 is overly restrictive but shouldn’t pose a problem, except that it does… If your temp path contains an unnecessary double backslash; C:\\temp for example rather that C:\temp then the first call will return the length including the double backslash and the second call will return the path with the unnecessary backslash removed. This means that for the example path shown above, the second call returns spaceRequired - 2 rather than the expected spaceRequired - 1 and the code fails with the rather useless error message.

The fixed version of the code doesn’t suffer from this problem.

_tstring GetTempPath()
{
   const DWORD spaceRequired = ::GetTempPath(0, 0);
  
   if (spaceRequired == 0)
   {
      const DWORD lastError = GetLastError();
  
      throw CWin32Exception(_T("GetTempPath()"), lastError);
   }
  
   _tstring directory;
  
   directory.resize(spaceRequired - 1);
  
   const DWORD spaceUsed = ::GetTempPath(
      spaceRequired,
      const_cast<tchar*>(directory.c_str()));
  
   if (spaceUsed == 0)
   {
      const DWORD lastError = GetLastError();
  
      throw CWin32Exception(_T("GetTempPath()"), lastError);
   }
  
   if (spaceUsed >= spaceRequired)
   {
      throw CException(
         _T("GetTempPath()"), 
         _T("Failed to get temp path, second call needed more space ") +
         ToString(spaceUsed + 1) + 
         _T(" than first call allocated ") +
         ToString(spaceRequired));
   }
  
   directory.resize(spaceUsed);
  
   return directory;
}

Whilst there are still, no doubt, style issues, this version checks for errors in a better way (I’d first thought that the failure was some kind of permissions thing) and reports these errors in a clearer manner. Hopefully it will be another 10 years at least before I get another error report for this piece of code.

This fix will be included in version 6.3 of The Server Framework code which currently has no scheduled release date.