Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 11 - the answers

What's wrong with this code, part 11 - the answers

  • Comments 5
Well, clearly the shorter the example, the quicker people pick up the problem.

The problem was:

    if (!CreateProcessW(NULL, PROCESS_NAME, NULL, NULL, FALSE, 0, NULL, NULL, &startupInfo, &processInformation))

The issue here is that you can't pass a constant string to CreateProcessW.  The documentation for CreateProcessW states:

The Unicode version of this function, CreateProcessW, will fail if this parameter is a const string.

But it doesn't explain why it fails.  Skywing pointed out what's going on - the developer who wrote CreateProcessW temporarily modified the input string to separate the executable name from the arguments.  There are two issues here.

The first is the obvious one: You can't use constant strings.  And there's a second one: You can't call CreateProcess with the same buffer on multiple threads.  While the call to CreateProcess restores the contents of the lpCommandLine string before it returns, there is still a period of time when the contents of lpCommandLine are not what was passed in originally.

So if you have two threads calling CreateFile with the same buffer, some of the calls to CreateProcess will have no command line arguments, and some will have the expected command line arguments (Hmmm.  Maybe I should have used this as the example instead of the one I did - to show a concurrency issue in the base API set.  Oh well...).

As I said, I got burned by this one, the reason for this is that the current compiler puts constant strings in a read-only data section.  And thus the PROCESS_NAME variable is read-only, and it causes the crash.

During the comments, I asked "But why did I say that it was possible that you won't see the bug?".  Well, it's because the bug depends on the compiler switches.  The default switches exhibit the bug.  But if you were using an older compiler (circa vc6, if I recall correctly) or if your compiler switches were set differently, the behavior would be different.

The relevent switch is the /Gf switch.  The /GF switch enables a compiler feature called string pooling - essentially constant strings are collapsed together if they're the same string.  If your code was compiled with the /Gf switch (enable string pooling as read/write strings), then the constant strings used for PROCESS_NAME would not be kept in a read-only section.

The reason that you see this the bug with the default compilation options is that the /ZI command line switch (which enables symbolic debugging in a PDB file) forces the /GF switch to be enabled.

The history of string pooling in NT is long and varied.  When NT 3.1 was under development, the person responsible for the builds changed the state of the /Gf flag several times during the lifetime of the project - there was code in the system that had depended on constant strings being read/write, and other code that depended on the lack of string pooling (you'd have two strings with the same text and modify one of the two strings).  I believe that NT 3.1 eventually shipped without the string pooling, and that it was added after we shipped (probably in the NT 3.51 timeframe, when we were concentrating on performance).

 

Kudos:

ParadoxLost nailed the base problem in the very first comment.  Mike Dunn nailed the constant string issue, and Purplet nailed the /Gf /GS switch issue.

Mea culpas:

Joel Spolsky and eruprahgp pointed out the possibility of a handle leak with the handles returned in the processInformation structure.  That's a fair criticism, but since the process exits immediately afterwards, the handles will be closed.  But in production systems this is a real issue.

gregg, Bradly Grainger, and Universalis pointed out that status is only assigned if CreateProcess fails -it's uninitialized otherwise.

Setfan Kuhr totally nailed a problem I'd not seen: The startupinfo doesn't have the wShowWindow and dwFlags fields - so notepad might not be visible when it's launched (I don't know if this is a problem or not - the process I was launching was a console process so it was irrelevant).

Major kudos to brantgurga for thinking to run this silly little program under appverifier.  That pointed out a couple of other issues: The code shouldn't have lpApplicationName as null and lpCommandLine as non null because of the spaces in file names issues.

And Adrian totally blew me away when he pointed out that on versions of NT before NT4 SP5, the GetFileAttributes API did not have a stable return value.  Wow, that's cool, I'd never known that.

  • Also, CreateProcess's STARTUPINFO parameter must be NULL on Windows CE, but must be non-NULL on the Windows desktop. In my code, I have a CreateProcess() wrapper function that hides this like so:

    STARTUPINFO* pStartupInfo = NULL;

    #ifndef UNDER_CE
    //
    // Windows CE does not support the STARTUPINFO paramter, but
    // Windows desktop requires a non-NULL STARTUPINFO parameter!
    //
    STARTUPINFO startupInfo;
    pStartupInfo = &startupInfo;
    GetStartupInfo(pStartupInfo);
    #endif // UNDER_CE

    PROCESS_INFORMATION processInfo;
    BOOL ok = CreateProcess(NULL, tCommandLine, NULL, NULL, FALSE, NULL, NULL, NULL, pStartupInfo, &processInfo);
  • Sorry I'm being too lazy to look at MSDN before replying...

    > If your code was compiled with the /Gf
    > switch (enable string pooling as read/write
    > strings), then the constant strings used for
    > PROCESS_NAME would not be kept in a read-
    > only section.

    If that's what it really does, isn't it backwards from what anyone would really need? If you don't have string pooling then whoever vandalizes one string constant vandalizes that one string constant. If you do have string pooling then you DO want them read-only, so that whoever tries to vandalize a string constant will crash themselves instead of succeeding in smashing other innocent victims.

    > the /ZI command line switch (which enables
    > symbolic debugging in a PDB file) forces
    > the /GF switch to be enabled.

    Oh, even more neat. If you want debugging information then you're forced to get a backwards option which obscures bugs by letting vandals succeed and making innocent victims crash.

    > When NT 3.1 was under development, the
    > person responsible for the builds changed
    > the state of the /Gf flag several times
    > during the lifetime of the project

    In contrast to that, quality code doesn't care if the /Gf flag is enabled and doesn't even care if the meaning of the /Gf flag gets fixed. During debugging programmers need options that will reveal bugs quickly, but after the bugs are gone the program no longer tries to do anything as silly as altering things that are allowed to be string constants.
  • Sorry for the previous comment. Now I've read it in MSDN, /GF and /Gf are not identical, and /Gf is properly going to stop existing.
  • As a self-titled language lawyer I have to say that the sentence "if this parameter is a const string" is IMHO quite wrong. The real problem with the function (that it _modifies_ the string) is only vaguely implied, while it should be the core of the warning. IF the sentence would read "...is a constant string...", it would be a bit better, even though still a bit foggy. But using a C keyword const makes the sentence a nonsense (IMHO). String literal in ANSI C is NOT a const string (although it is undefined behavior to modify the resulting array).
  • My last post on contracts introduced the idea that a languages type system can be used as a mechanism

Page 1 of 1 (5 items)