Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

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

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

  • Comments 25

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.

  • 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)
  • You don't close the handles returned in startupInfo and processInformation?
  • 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...
  • 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
  • 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.
  • maybe not important in this tester, but status is only assigned a value if CreateProcessW returns FALSE.
  • 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.
  • 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.
  • That's weird; I didn't see existing comments when posting my own (repetitive) one.
  • It's possible to specify for initialized data (.data by default) segment to be read-only.
  • HANDLE LEAK!!!!!
    processInformation.hProcess and processInformation.hThread
    are not closed.
  • 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 Files\something\something.exe", not "C:\Program.exe" with arguments "Files\something\something.exe" which it would be without quotes. Both of those issues are subtle security problems.
  • 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
  • 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
  • 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. ;-)
Page 1 of 2 (25 items) 12