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.

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s

This site uses Akismet to reduce spam. Learn how your comment data is processed.