wild pointers in current named pipe implementation?

Dan Kegel dank at kegel.com
Tue Apr 15 12:12:18 CDT 2003


Bill Medland wrote:
> 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));
>>}
> 
> But that is not what I am fixing. 

All I was saying is that the above is a minimal test case
for the problem your patch fixes.  Without your patch or something like it,
wineserver crashes for me.

> /*
>  * 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.
>  */
>     h = CreateNamedPipe ("\\\\.\\pipe\\tstpipe",
>                          PIPE_ACCESS_DUPLEX,
>                          PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT,
>                          1,
>                          512,
>                          512,
>                          60000,
>                          NULL))
>     ConnectNamedPipe (h, NULL)
>     ReadFile (h, &req, sizeof(req), &num_read, NULL)
>     WriteFile (h, &reply, sizeof(reply), &num_written, NULL)
>     /* Deliberately do not flush; this is what we want to test
>      */
>     DisconnectNamedPipe (h);
>     CloseHandle (h);
> 
> ---
> /*
>  * 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.
>  */
>    h = CreateFile ("\\\\.\\pipe\\tstpipe",
>                          GENERIC_READ | GENERIC_WRITE,
>                          0, NULL, OPEN_EXISTING, 0,
>                          NULL))
 >    WriteFile (h, &req, sizeof(req), &num_written, NULL)
> 
>     /* 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;
> }

Thanks for posting the code.  So you're checking to make sure
that data doesn't get lost on a DisconnectNamedPipe?

> 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 agree in general, but looking at your patch, it appears
to cause FlushFileBuffers to return an error when the object
has no fds.
At least in my late night quick test on win2K, it's quite ok to
call FlushFileBuffers on an object that in general have fds
but doesn't have one at the moment.

It may take some rejiggering to fix this without putting
pipe-specific code where it doesn't belong.  At the moment,
I think you can't call the object's flush routine
until you have an fd, which means the object doesn't get
a chance to say "Oh, ok, it's a flush request when I don't
have an fd, no error".

But then as I've said earlier, I hardly understand this code,
so I should just keep quiet :-)

> 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).

Mike isn't doing flushing code for this version of the named
pipe implementation, as it's not clear it's possible without a rewrite.
He's replacing the whole thing.
- Dan

-- 
Dan Kegel
http://www.kegel.com
http://counter.li.org/cgi-bin/runscript/display-person.cgi?user=78045




More information about the wine-devel mailing list