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.

  • Adrian: my current PlatSDK says
    #define INVALID_FILE_ATTRIBUTES ((DWORD)-1)
    in winbase.h so I don't think this is the problem.

    --
    Stefan
  • 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!)
  • Stefan, the GetFileAttributesW check is there to catch the "windows installed in something other than c:\windows" bug.
  • >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...
  • 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[]...)
  • > 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.
  • 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
  • > 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 :)
  • 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.
  • Once upon a time, I watched the TV show The Paper Chase. As I recall it, the feared and respected professor

Page 2 of 2 (25 items) 12