Created attachment 118928 [details]
Test code.

This is the (very non-cleaned up) test code. It can run both from the console and as a service. 

From the console, just run "spawn -c". 

To run as a service, first install it ("spawn -i"), then run it ("net start spawntest"). Starting the service will fail, but it should run the c:\temp\test.bat file a few times before failing; I have that script writing to a log file to make sure it's been invoked correcly. 

To uninstall the service, just do "spawn -u", unless you like SCM error messages when you boot up. :-)

Created attachment 118929 [details] [review]
Add new start flag.

This just adds a flag to implement the feature in bug 509201.

Created attachment 118930 [details] [review]
gspawn-win32.c changes

These are the main changes, against the gspawn-win32.c file that shipped with glib 2.18.0. With these changes, there's no need for the helper application anymore.

I think I covered all the available spawn flags to the best of my knowledge / understanding.

Also, I'm not very used to the glib coding conventions, but I tried to stay in line with the rest of the file.

Created attachment 118932 [details] [review]
gspawn-win32.c changes

Sorry, just noticed I left some stuff that can go away in the previous patch.

Hi, any chance of someone taking a look at this for the current unstable release? Thanks!

> The gspawn helper process doesn't seem to work too well. Running a 
> console app against glib 2.18 (both downloaded from the website and
> custom compiled with VC8), I get a "failed to read from pipe"
>  error when trying to spawn a process. 

That is maybe some bug then that needs to be fixed.

By the way, your patch doesn't apply against the current trunk glib. Could you please update it if you feel you still want to try to get it in? I am not optimistic about the chances, though, see below.

> I took the time to try to rewrite the functions around the
> Win32 CreateProcess() function, instead of relying on 
> the MS CRT functions / helper process,

Sorry, I don't think that is an entirely good idea. That will then (as you say in your comments) mean that much of the semantics of the g_spawn functions is not implemented at all.

The worst thing is that with your code, C file descriptors are not inherited at all, as far as I can see. (Except perhaps stdin if requested with G_SPAWN_CHILD_INHERITS_STDIN, if we assume that GetStdHandle(STD_INPUT_HANDLE) is always the same as _osf_gethandle(fileno(stdin)). Is it?)

Instead, what would be better, would be to add more Windows-specific documentation for these functions. We should clearly tell people who are writing code that needs to work perfectly also on Windows, and are prepared to spend some effort on that, that they really should use Windows-specific code in their application if necessary. For instance, in my opinion writing code that is intended to be runnable as a Windows Service clearly falls into this category.

What could be done would be to add some new Windows-specific g_win32_spawn_*() function that would be less POSIX-inspired and closer to CreateProcess() in semantics. For instance fully usable in services, but still offer a more GLib-like API that the bare CreateProcess() (like it would obviously use UTF-8). Such a new function would indeed use CreateProcessW() directly, and it would need to be clearly documented that C file handles are not inherited.

(In reply to comment #6)
> > The gspawn helper process doesn't seem to work too well. Running a 
> > console app against glib 2.18 (both downloaded from the website and
> > custom compiled with VC8), I get a "failed to read from pipe"
> >  error when trying to spawn a process. 
> 
> That is maybe some bug then that needs to be fixed.

I tried to figure that one out but couldn't find anything wrong with the code... maybe I should give it another try.

> Sorry, I don't think that is an entirely good idea. That will then (as you say
> in your comments) mean that much of the semantics of the g_spawn functions is
> not implemented at all.
> 
> The worst thing is that with your code, C file descriptors are not inherited at
> all

To be fair, the only two flags that are not handled in my patch are G_SPAWN_SEARCH_PATH (CreateProcess() doesn't really have anything to *not* search the path AFAIK) and G_SPAWN_LEAVE_DESCRIPTORS_OPEN.

I understand the desire to maintain the current functionality, even though the lack of fork() makes it a real pain to use inherited fds (and the spawn helpers have hacks of their own like the hardcoded limit of 1000 when closing fds - unless that has changed recently).

I tried to figure out a way to inherit fds using CreateProcess but I've given up at this point...

> GetStdHandle(STD_INPUT_HANDLE)
> is always the same as _osf_gethandle(fileno(stdin)). Is it?)

Haven't written any code to make sure, but the documentation says that's the case.

> Instead, what would be better, would be to add more Windows-specific
> documentation for these functions. We should clearly tell people who are
> writing code that needs to work perfectly also on Windows, and are prepared to
> spend some effort on that, that they really should use Windows-specific code in
> their application if necessary. 

This is not a case of writing code to run perfectly on Windows; it's a case of me trying to use an existing glib API and it not working at all. I don't really care about any of the Windows-specific features other than the flag to hide the console (which I don't know if it is possible to implement with the CRT alone), but in my environment the g_spawn functions are just plain not working... :-/

> For instance, in my opinion writing code that
> is intended to be runnable as a Windows Service clearly falls into this
> category.

I remember you using this argument in some other discussion, and I really do not agree with it. Other than the initial SCM handshake code, which is pretty small, there's nothing necessarily Win32-specific in writing a Win32 service. It's an application like any other.

> What could be done would be to add some new Windows-specific g_win32_spawn_*()
> function that would be less POSIX-inspired and closer to CreateProcess() in
> semantics.

I'm not against that, but I really would like to figure out why the g_spawn functions are not working for me in the first place. I'll see if I find some time to take a closer look at all the g_spawn_helper code...

I think I found out why the child process is having problems. It's because of the code that's closing all the fds blindly; the code itself is fine but the default behavior of the CRT is to assert if the input parameters are invalid (e.g., an invalid fd being closed; it asserts instead of setting EBADF).

To fix it, see:

http://msdn.microsoft.com/en-us/library/a9yf33zb(VS.80).aspx
(Linked from http://msdn.microsoft.com/en-us/library/ksazx244(VS.80).aspx, linked from http://msdn.microsoft.com/en-us/library/5fzwd5ss(VS.80).aspx .)

I added a dumb handler that does nothing to gspawn-win32-helper.c and now my child processes run fine; still have to see if I figure out how to hide the console window. :-/

> I tried to figure out a way to inherit fds using CreateProcess
> but I've given up at this point...

Unfortunately, how this happens is not publicly documented (as far as I know), even if it is actually rather trivial. Look in the sources of the Microsoft C library, the dospawn.c file.

The Microsoft C library sources (at least, most of them) are distributed with their Visual C++/Studio/Whatever product. At least the editions that cost money, I don't know if they are part of the no-cost "Express" editions. The sources used to be distributed also with the Platform SDK, but in the more current Windows SDK (which has superseded the Platform SDK) they don't seem to be included any more.

I don't know whether Microsoft considers the details of how C file descriptors (i.e., the mapping from C file descriptor numbers, which are small integers, to file HANDLEs) are inherited a trade secret or something like that. But I guess I can say that it's the undocumented cbReserved2 and lpReserved2 fields in the STARTUPINFO struct that are used.

Still, anyway, even that isn't enough. The C library code also takes care to not inherit those file descriptors that have been created as noninheritable, with the _O_NOIHERIT flag to open() or _pipe(). Unfortunately it is not possible, as far as I know, to find out the inherit-or-not flag for a file descriptors from outside the C library. So to make sure C level file descriptor are inherited propertly, one has to use the C library's mechanisms to start a child process.

Created attachment 130721 [details] [review]
patch to "disable" CRT argument checking

I don't know if #ifdefs might be need to compile with mingw - I only have MSVC around.

Hmm, so a simple close(fd) where fd is not an open file descriptor, in the modern compiler-specific Microsoft C libraries, doesn't simply set errno to EBADF and return -1, but instead eventually calls UnhandledExceptionFilter() which causes a GUI error message.

Wow, that is totally broken. One more reason to be happy that mingw sticks to msvcrt.dll. And apparently there is no way to even check beforehand if an integer is a valid file descriptor or not, as fstat() and even _get_osfhandle() seem to do the same checks that cause ?

The call to set a dummy invalid parameter handler should be in some #if defined(_MSC_VER) && _MSC_VER >= 1500 I guess.

I don't remember in which version this stuff was added, and MSDN is not clear about it either. I remember seeing mentions of "secure CRT functions" back in VS 2003 but don't have it available to check.

Hi Marcelo,

Sorry if this came later than you've expected.

This problem was indeed a long-standing problem that affected GLib, and tools/programs that used g_spawn* on Windows built with Visual Studio 2005 and later (!-I build and test GLib with Visual Studio 2008, 2010 and 2012 at this time), which includes tools that were later introduced to GLib, such as glib-compile-resources (it used the g_spawn_* APIs to call xmllint and gdk-pixbuf-pixdata to generate source files and resource binary blobs).

The issue was fixed in commit c7996825 (bug 693646)-basically what was done was to store up the fd's that were opened during the call of g_spawn_* on Windows and run through the list at the end to close the fd's, in conjunction with using the (empty) invalid parameter handler which you have mentioned in http://msdn.microsoft.com/en-us/library/a9yf33zb.aspx, which you have mentioned.

I will close this bug now, as a duplicate of bug 693646 as a result.

With blessings, thank you for the report and patience on this!

*** This bug has been marked as a duplicate of bug 693646 ***