What’s wrong with this code–a real world example

I was working on a new feature earlier today and I discovered that while the code worked just fine when run as a 32bit app, it failed miserably when run as a 64bit app.

If I was writing code that used polymorphic types (like DWORD_PTR) or something that depended on platform specific differences, this wouldn’t be a surprise, but I wasn’t.

 

Here’s the code in question:

         DWORD cchString;
        DWORD cbValue;
        HRESULT hr = CorSigUncompressData(pbBlob, cbBlob, &cchString, &cbValue);
        if (SUCCEEDED(hr))
        {
            cbBlob -= cbValue;
            pbBlob += cbValue;

            if (cbBlob >= cchString)
            {
                //  Convert to unicode
                wchar_t rgchTypeName[c_cchTypeNameMax];
                DWORD cchString = MultiByteToWideChar(CP_UTF8, 0, reinterpret_cast<LPCSTR>(pbBlob), static_cast<int>(cchString), 
                                                      rgchTypeName, ARRAYSIZE(rgchTypeName));
                if (cchString != 0 && cchString < ARRAYSIZE(rgchTypeName))
                {
                    //  Ensure that the string is null terminated.
                    rgchTypeName[cchString] = L'\0';
                }
                cbBlob -= cchString;
                pbBlob += cchString;
            }
        }

This code parses a ECMA 335 SerString. I’ve removed a bunch  of error checking and other code to make the code simpler (and the bug more obvious).

When I ran the code when compiled for 32bits, the rgchTypeName buffer contained the expected string contents. However, when I ran it on a 64bit compiled binary, I only had the first 6 characters of the string. Needless to say, that messed up the subsequent parsing of the blob containing the string.

Stepping into the code in the debugger, I saw that the cchString variable had the correct length at the point of the call to MultiByteToWideChar, however when I stepped into the call to MultiByteToWideChar, the value had changed from the expected length to 6!

After a couple of minutes staring at the code, I realized what was happening: Through a cut&paste error, I had accidentally double-declared the cchString local variable.  That meant that the cchString variable passed to the MulitByteToWideChar call was actually an uninitialized local variable, so it’s not surprising that the string length was bogus.

So why did this fail on 32bit code but not on 64bit?  Well, it turns out that the 32bit compiler’s code generation algorithm had re-used the stack storage for the inner and outer cchString variables (this is safe to do because the outer cchString variable was not used anywhere outside this snippet), thus when the processor pushed the unitialized cchString variable it happened to push the right value.  Since the 64bit compiler allocates local variables differently, the uninitialized variable was immediately obvious.