On the perils of assuming file path manipulation is easy

I recently worked on a bug in which a product developed by my employer was no longer finding user settings files. Everything worked correctly in a prior version of the product but failed in the current version.

The following is a simplified version of the old code.

BOOL GetUserProfilePath(LPWSTR profilePathName)  
{  
    if (profilePathName == nullptr) return FALSE;  
    profilePathName[0] = static_cast<wchar_t>(0);  
    std::wstring userPath = GetUserPath();  
    if (userPath.empty()) return FALSE;  
    TCHAR tempPath[MAX_PATH];  
    GetProgramPath(tempPath);  
    ::PathAppend(tempPath, userPath.c_str());  
    ::PathCanonicalize(profilePathName, tempPath);  
    if (profilePathName[0])  
    {  
        return TRUE;  
    }  
    return FALSE;  
}  

The following is a simplified version of the new code.

BOOL GetUserProfilePath(std::wstring& profilePathName)  
{  
    profilePathName.clear();  
    std::wstring userPath = GetUserPath();  
    if (userPath.empty()) return FALSE;  
    wstring tempPath;  
    GetProgramPath(tempPath);  
    Path::Append(tempPath, userPath);  
    wchar_t temp[MAX_PATH];  
    PathCanonicalize(temp, tempPath.c_str());  
    profilePathName = temp;  
    if (!profilePathName.empty())  
    {  
        return TRUE;  
    }  
    return FALSE;  
}  

At first glance these two implementations appear to be equivalent.

Now, here are a few additional details that reveal why they are not in any way equivalent.

First, due to a previously unknown bug, the GetUserPath function returned a complete path, not a relative path. It was, in fact, the program path. Thus if the program path was "c:\MyApp," then the value returned by the GetUserPath function was "c:\MyApp."

Second, the GetProgramPath function also obtains the program path, thus in this example its value would also be "c:\MyApp."

Thus, when the ::PathAppend Win32 API function was being called it was being asked to append a full path onto another full path. Microsoft chose to, in their infinite wisdom, attempt to protect you from this mistake by trying to *just do the right thing* by generating a valid path despite your mistake. Thus, the bug in the GetUserPath function was harmless.

The person who wrote the Path::Append function was not aware of this. The following was their implementation of the function.

namespace Path  
{  
    // Various details left out.  
    bool Append(std::wstring& dest, const std::wstring& source)  
    {  
        Path::AddBackslash(dest);  
        dest += source;  
        return !dest.empty();  
    }  
}  

Therefore, the end result of this change was that the otherwise harmless bug in the GetUserPath function suddenly became a big deal. Before this change, the GetUserProfilePath function returned the string "c:\MyApp" regardless of the bug in the GetUserPath function. After this change the GetUserProfilePath function returned the string "c:\MyApp\c:\MyApp," which is an invalid path.

I was asked to fix this with the smallest change possible. The GetUserPath function happens to be part of a deprecated component that we are trying to phase out. Therefore, I was not allowed to touch it. As a result, I chose to fix it by simply modifying the Path::Append function so that it uses the ::PathAppend Win32 API function instead of attempting to do the work itself.

I am sharing this in the hopes that it could spare you the difficulties I had over the three days it took me to diagnose and fix this bug.