Re: wild pointers in current named pipe implementation?

[Bill Medland]

On April 14, 2003 10:34 pm, Dan Kegel wrote:
> In fact, here's the smallest program that reproduces the
> problem for me:
>
> #include <windows.h>
>
> main()
> {
>      FlushFileBuffers(CreateNamedPipeA("\\.\PiPe\foo",
>          PIPE_ACCESS_DUPLEX, PIPE_TYPE_BYTE|PIPE_WAIT,
>          1, 1024, 1024, NMPWAIT_USE_DEFAULT_WAIT, NULL));
> }
>
> I believe this will run fine on real Windows, and that
> both calls will succeed there.  I verified that FlushFileBuffers
> returns nonzero on Win2K even when calling it on a just-created
> pipe nobody had ever connected to.  Thus I think Bill's patch
> is probably a bit wrong; if fd is NULL, it should just say
> "OK, I flushed" rather than returning a windows error.
> - Dan

But that is not what I am fixing.  That should be handled by FlushFileBuffers 
handling the errors appropriately.

What I am handling is the following (for which it IS correct)

/*
 * namedp/server.c
 *
 * Testbed server to find out what error is returned when reading from a pipe
 * that has been broken by the server closing it.
 */

#include <stdio.h>
#include <windows.h>

int main (int argc, char **argv)
{
    HANDLE h;
    DWORD req, reply = 54321;
    DWORD num_written, num_read;

if (INVALID_HANDLE_VALUE == (h = CreateNamedPipe ("\\.\pipe\tstpipe",
                         PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT,
                         1,
                         512,
                         512,
                         60000,
                         NULL)))
    {
        fprintf (stderr, "Error %lx on CreateNamedPipe\n", GetLastError());
        return 1;
    }
    if (!ConnectNamedPipe (h, NULL))
    {
        DWORD error = GetLastError();
        if (error != ERROR_PIPE_CONNECTED)
        {
            fprintf (stderr, "Error %lx on ConnectNamedPipe\n", error);
            CloseHandle (h);
            return 1;
        }
    }
    if (!ReadFile (h, &req, sizeof(req), &num_read, NULL) || num_read != 
sizeof(&req))
    {
        fprintf (stderr, "Error %lx or incomplete %d cf %d on ReadFile\n", 
GetLastError(), num_read, sizeof(req));
        DisconnectNamedPipe (h);
        CloseHandle (h);
        return 1;
    }
    if (!WriteFile (h, &reply, sizeof(reply), &num_written, NULL) || 
num_written != sizeof (reply))
    {
        fprintf (stderr, "Error %lx or incomplete %d cf %d on WriteFile\n", 
GetLastError(), num_written, sizeof (reply));
        DisconnectNamedPipe (h);
        CloseHandle (h);
        return 1;
    }
    /* Deliberately do not flush; this is what we want to test
     */
    DisconnectNamedPipe (h);
    CloseHandle (h);
    return 0;
}

---
/*
 * namedp/client.c
 *
 * Testbed client to find out what error is returned when reading from a pipe
 * that has been broken by the server closing it.
 */

#include <stdio.h>
#include <windows.h>

int main (int argc, char **argv)
{
    HANDLE h;
    int retry;
    DWORD req = 12345, reply;
    DWORD num_written, num_read;

for (retry = 100; retry; retry--)
    {
        if (INVALID_HANDLE_VALUE != (h = CreateFile ("\\.\pipe\tstpipe",
                         GENERIC_READ | GENERIC_WRITE,
                         0, NULL, OPEN_EXISTING, 0,
                         NULL))) break;
        Sleep (10);
    }
    if (!retry)
    {
        fprintf (stderr, "Failed to open the pipe\n");
        return 1;
    }
    if (!WriteFile (h, &req, sizeof(req), &num_written, NULL) || num_written 
!= sizeof(&req))
    {
        fprintf (stderr, "Error %lx or incomplete %d cf %d on WriteFile\n", 
GetLastError(), num_written, sizeof(req));
        CloseHandle (h);
        return 1;
    }

/* This is where we want to hang around for the server to complete */
    Sleep (20000);

if (!ReadFile (h, &reply, sizeof(reply), &num_read, NULL) || num_read != 
sizeof (reply))
    {
        fprintf (stderr, "Error %lx or incomplete %d cf %d on ReadFile\n", 
GetLastError(), num_read, sizeof (reply));
        CloseHandle (h);
        return 1;
    }
    CloseHandle (h);
    fprintf (stdout, "Reply was %lu\n", reply);
    return 0;
}

The point, as far as I am concerned, is that there is an error in the use of 
the "get object's fd" protocol.  The protocol has to handle three different 
situations:
1. The request is being made of an object that doesn't have fds; the request 
should never have been made in the first place.  (For example a process).  
Note that no_get_fd() already sets the last error so there is no need for 
get_handle_fd_obj to do so.
2. The request is being made of an object that does have fds and at the time 
of the request it actually has one.  That is reasonable; each object knows 
where it stored it and its "get fd" function can return it.
3. The request is being made of an object that does have fds in general, but 
doesn't actually have one at this moment.  That was the problem in the named 
pipe code.

Now clearly (since the error "ERROR_PIPE_NOT_CONNECTED" (or whatever it was) 
is pipe-specific) we must leave it to the object to set the error, not to the 
general fd code.  (I would hate to see code in fd.c that actually knew what 
sort of object it was looking at).

That is my justification for the fix that I submitted; I presume other people 
(Mike especially)  are already doing the flushing code, which clearly has to 
be quite specialised since it is blocking on the client's reading of the 
pipe.  Clearly that code will have to handle flushing a handle that does not 
yet have a fd on it.  (I haven't looked but I presume that the fd gets added 
when the pipe is connected).