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:
- The code is unicode-only. It is not intended to work in non unicode environments.
- 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
- Anonymous
April 14, 2005
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) - Anonymous
April 14, 2005
You don't close the handles returned in startupInfo and processInformation? - Anonymous
April 14, 2005
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... - Anonymous
April 14, 2005
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 - Anonymous
April 14, 2005
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. - Anonymous
April 14, 2005
maybe not important in this tester, but status is only assigned a value if CreateProcessW returns FALSE. - Anonymous
April 14, 2005
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. - Anonymous
April 14, 2005
The comment has been removed - Anonymous
April 14, 2005
That's weird; I didn't see existing comments when posting my own (repetitive) one. - Anonymous
April 14, 2005
It's possible to specify for initialized data (.data by default) segment to be read-only. - Anonymous
April 14, 2005
HANDLE LEAK!!!!!
processInformation.hProcess and processInformation.hThread
are not closed. - Anonymous
April 14, 2005
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. - Anonymous
April 14, 2005
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 - Anonymous
April 14, 2005
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 - Anonymous
April 14, 2005
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. ;-) - Anonymous
April 14, 2005
Adrian: my current PlatSDK says
#define INVALID_FILE_ATTRIBUTES ((DWORD)-1)
in winbase.h so I don't think this is the problem.
--
Stefan - Anonymous
April 14, 2005
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!) - Anonymous
April 14, 2005
Stefan, the GetFileAttributesW check is there to catch the "windows installed in something other than c:windows" bug. - Anonymous
April 14, 2005
>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... - Anonymous
April 14, 2005
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[]...) - Anonymous
April 14, 2005
> 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. - Anonymous
April 14, 2005
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 - Anonymous
April 14, 2005
The comment has been removed - Anonymous
April 14, 2005
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. - Anonymous
January 04, 2007
Once upon a time, I watched the TV show The Paper Chase. As I recall it, the feared and respected professor