---- Reported by lluis@ximian.com 2005-08-06 14:05:36 MST ----

I found this problem while tracking down a failure in MonoDevelop when
starting a process. In some cases, this call in io.c:

	ret=pipe (filedes);

returns in filedes a file descriptor with the value zero. The return value
ret is != -1, so I guess that zero is a valid value for a file descriptor.
However, the call to _wapi_handle_new_fd() returns NULL for this fd (since
it just returns GUINT_TO_POINTER(fd)), and I think this is not correct
because NULL should not be a valid value for a handle (at least, it is not
in WIN32).

The process later fails to start because in process.c:857 there is a call
to CloseHandle(procinfo.hThread), and procinfo.hThread happens to be
initialized to NULL, and this accidentally closes the pipe.

My particular problem can be solved by removing that CloseHandle call
(which is useless right now), although I think something must be done to
avoid returning null handles.



---- Additional Comments From miguel@ximian.com 2005-08-23 13:35:59 MST ----

Alternatively, we could check if the return is zero, if so we dup the
fd and close the zero fd.

A hack





---- Additional Comments From miguel@ximian.com 2006-10-07 11:02:24 MST ----

I tried the approach I suggested, but it breaks when we have to cope
with the real handle "fd 0" (for Console), and sprinkling ifs
everywhere to cope with the 0 case sounds like a bad idea, and it
woudld be easy to introduce this error in future uses of the code if
we are not careful.

I saw that the code in process.c no longer calls CloseHandle if the
value is NULL.

But I think the real fix is not to set the value to NULL, but to
initialize the value of hThread to INVALID_HANDLE_VALUE (-1), which
btw, we do not seem to use at all in Mono's io-layer, so this value
might be fine.

The following patch implements this:

Index: handles.c
===================================================================
--- handles.c   (revision 66360)
+++ handles.c   (working copy)
@@ -699,7 +699,7 @@
                              gboolean search_shared)
 {
        struct _WapiHandleUnshared *handle_data = NULL;
-       struct _WapiHandleShared *shared;
+       struct _WapiHandleShared *shared = NULL;
        gpointer ret = NULL;
        guint32 i, k;
        gboolean found = FALSE;
@@ -1222,6 +1222,10 @@
                        return(FALSE);
                }
        }
+       if (handle == _WAPI_HANDLE_INVALID){
+               SetLastError (ERROR_INVALID_PARAMETER);
+               return(FALSE);
+       }

        _wapi_handle_unref (handle);

Index: processes.c
===================================================================
--- processes.c (revision 66360)
+++ processes.c (working copy)
@@ -818,7 +818,7 @@
                /* FIXME: we might need to handle the thread info some
                 * day
                 */
-               process_info->hThread = NULL;
+               process_info->hThread = INVALID_HANDLE_VALUE;
                process_info->dwThreadId = 0;
        }




---- Additional Comments From dick@ximian.com 2006-10-07 16:42:54 MST ----

This is meant to happen, it's not a bug.

It's because we map file descriptors to handles directly.  I know
windows doesn't return NULL handles, but there's no getting around it
on linux and still keep the fd mapping feature that I didn't want to
implement in the first place anyway :)

Miguel's patch looks good; the real bug is in process.c.




---- Additional Comments From miguel@ximian.com 2006-11-11 14:19:20 MST ----

My improved patch is on 67720.

I vote for closing this bug for now, Dick?



---- Additional Comments From dick@ximian.com 2006-11-12 08:57:32 MST ----

I'll have a look for NULL handle returns next week and close this bug
then.



---- Additional Comments From dick@ximian.com 2006-11-13 10:34:35 MST ----

NULL is the proper error return for Event, Mutex, Semaphore and Thread
creation functions, so we just need to be diligent about checking
return conditions.  I've added some checks in the runtime that were
missing, so closing this bug.


Unknown operating system NLD 9. Setting to default OS "Other".