What’s wrong with this code, part 11: Launching notepad.


Anyway, I ran into this problem while I was writing some stuff at work.  I’ve restructured it to remove the “work-ness” of the code, but the problem still exists.

It’s short, but sweet:

#include <stdio.h>
#include <windows.h>

#define PROCESS_NAME L"C:\\WINDOWS\\NOTEPAD.EXE"
int _tmain(int argc, _TCHAR* argv[])
{
    PROCESS_INFORMATION processInformation = {0};
    STARTUPINFO startupInfo = {0};
    DWORD status;

    if (GetFileAttributesW(PROCESS_NAME) == INVALID_FILE_ATTRIBUTES)
    {
        wprintf(L"Can't find " PROCESS_NAME L"; error %d, aborting\n", GetLastError());
        return(1);
    }

    startupInfo.cb = sizeof(startupInfo);

    if (!CreateProcessW(NULL, PROCESS_NAME, NULL, NULL, FALSE, 0, NULL, NULL, &startupInfo, &processInformation))
    {
        status = GetLastError();
    }

    wprintf(L"Status launching " PROCESS_NAME L" is %d\n", status);

    return 0;
}

Some things that are NOT wrong with this code:

  1. The code is unicode-only.  It is not intended to work in non unicode environments.
  2. The code expects that the system directory is C:\WINDOWS.  This is not a bug, it’s by design.  The real code was more complicated this simplified version, but both show the same bug.

It’s possible that you won’t see the bug if you take it and compile it.  But it is still there, and if you compile it in the NT build environment, you’ll see the problem (you may also see the problem with current versions of Visual Studio, I was able to reproduce the problem quite easily.

Comments (25)

  1. paradoxlost says:

    the second param to CreateProcessW can’t to be a const.

    you should pass the EXE as the first param and any command switches as the second (non-const)

  2. Joel Spolsky says:

    You don’t close the handles returned in startupInfo and processInformation?

  3. paradoxlost: you nailed it. CreateProcessW modifies the 2nd parameter passed in (by putting a null at the end of the executable name), which means that the process name can’t be a constant string. But why did I say that it was possible that you won’t see the bug?

    Joel, I hadn’t thought about the handles in the startupInfo and processInfo – but they’re closed when the process exits so…

  4. paradoxlost says:

    it doesn’t show up during compile because there isn’t any outside code trying to modify the value, and compilers generally don’t have issues passing string literals to [w]char* type params

  5. Skywing says:

    Because the default compiler options treat string literals as a char*/wchar_t* and not a const char*/const wchar_t*.

    I assume this requirement is because someone took shortcuts in the implementation of CreateProcessW and didn’t allocate their own buffer. The reason why you can pass a constant string for CreateProcessA is because kernel32 already has to allocate its own buffer when thunking to CreateProcessW, which is by default non-const.

  6. gregg says:

    maybe not important in this tester, but status is only assigned a value if CreateProcessW returns FALSE.

  7. Skywing, actually that’s not quite the case – but you’re on the right track. Look at the results of cl /? for more info.

    And you’re right – someone took a shortcut in CreateProcessW. And you’re 100% right on the CreateProcessA aspect.

  8. Bradley Grainger says:

    The second parameter to CreateProcessW is of type LPWSTR, not LPCWSTR, so a constant string cannot be passed. PROCESS_NAME should be copied to a modifiable buffer, and a pointer to that buffer passed to CreateProcessW.

    Also, status is never initialised, so the last line may read uninitialised memory.

  9. Bradley Grainger says:

    That’s weird; I didn’t see existing comments when posting my own (repetitive) one.

  10. Dmitriy says:

    It’s possible to specify for initialized data (.data by default) segment to be read-only.

  11. eruprahgp says:

    HANDLE LEAK!!!!!

    processInformation.hProcess and processInformation.hThread

    are not closed.

  12. brantgurga says:

    There are a few more problems I see based on what Microsoft’s Application Verifier checks: lpApplicationName cannot be NULL, though not an issue since there are no spaces in the path used, there should generally be quotation marks around the command string since most paths today are in Program Files. You want "C:Program Filessomethingsomething.exe", not "C:Program.exe" with arguments "Filessomethingsomething.exe" which it would be without quotes. Both of those issues are subtle security problems.

  13. Stefan Kuhr says:

    Hi Larry,

    people could do weird things like erasing notepad.exe and replacing it with a directory named notepad.exe. Dunno if this is possible with WFP but it will definitely pass the test with GetFileAttributes.



    Stefan

  14. Stefan Kuhr says:

    Hi Larry,

    isn’t the test with GetFileAttributes completely pointless? Just inbetween testing for the existence for notepad, the thread could be preempted, notepad.exe be deleted by someone else and CreateProcess then be executed for a now non-existing file? Wouldn’t CreateProcess do the same tests (internally) as GetFileAttributes before?



    Stefan

  15. Adrian says:

    Everyone’s homing in on CreateProcessW, but I got stuck before that. First I looked up GetFileAttributes in the index for MSDN Platform SDK to see if you were checking the error condition correctly.

    Interestingly, that function is not listed in the index. There *is* such a function documented for Windows CE, but that one doesn’t say anything about the INVALID_FILE_ATTRIBUTES return value. (It suggests 0xFFFFFFFF instead.)

    Although my Platform SDK is up to date, I decided to search MSDN online just in case. Bingo! I got a link that looked good, but following it led to a "Location Cannot Be Found" page.

    So I’d say the bug is that you’re trying to use a Windows CE-only function on Windows NT. 😉

  16. Stefan Kuhr says:

    Adrian: my current PlatSDK says

    #define INVALID_FILE_ATTRIBUTES ((DWORD)-1)

    in winbase.h so I don’t think this is the problem.



    Stefan

  17. Adrian says:

    The return value from GetFileAttributes is not reliable before NT 4.0 SP 5. It can return a successful code even when the file isn’t there.

    http://support.microsoft.com/default.aspx?scid=kb;en-us;193763

    (Yeah, I know. NT 4.0 is old, but lots of us still have to support Windows 95!)

  18. Stefan, the GetFileAttributesW check is there to catch the "windows installed in something other than c:windows" bug.

  19. Mike Dunn says:

    >But why did I say that it was possible that you won’t see the bug?

    Not sure anyone has hit on this point yet, but the compiler can allocate string literals in read-only memory, so when CreateProcess() tries to write that 0 character, the app will crash.

    This question comes up now and again on boards, and I’m surprised that people are wondering why writing to a *literal* does something bad…

  20. Just a nitpick, but you said the program was designed as UNICODE _only_. Shouldn’t you use the UNICODE main instead then?

    Like switching:

    int _tmain(int argc, _TCHAR* argv[]) with

    int wmain(int argc, wchar_t* argv[]) (or LPWSTR argv[]…)

  21. Arta says:

    > But why did I say that it was possible that you won’t see the bug?

    Alignment? Is the literal is padded to bring its length up to whatever alignment the compiler is using, so there’s enough space for CreateProcessW to append the null? Thus, if the alignment changes, a previously ‘hidden’ bug would be revealed.

  22. Stefan Kuhr says:

    Hi Larry,

    could it be that notepad will not be started as a visible window because we did not correctly fill out the STARTUPINFO’s members wShowWindow, dwFlags and lpDesktop?



    Stefan

  23. Purplet [italy] says:

    > But why did I say that it was possible that you won’t see the bug?

    I don’t know how the compiler allocates the PROCESS_NAME data in memory, so forgive me any stupidity 😉

    First I think there are probably three instances of PROCESS_NAME in memory because it is a define – or at least there are if certain compiler options are checked or not – the string pooling options.

    String pooling options help :

    "These options enable the compiler to create a single copy of identical strings in the program image and in memory during execution, resulting in smaller programs, an optimization called string pooling.

    /Gf pools strings as read/write.

    /GF pools strings as read-only. "

    So, imho, if the flag is /Gf (or maybe if the strings are not even pooled), the bug does not occur. Viceversa, /GF make the exception appear 🙂

  24. Universalis says:

    I know this is not as celestial as the other comments, in fact it’s rather mere but…

    Wouldn’t it have been politer to initialise ‘status’? At present you only set its value if there was an error, so wprintf() will print a nonsensical number if Notepad is launched successfully.

  25. Once upon a time, I watched the TV show The Paper Chase. As I recall it, the feared and respected professor