wild pointers in current named pipe implementation?

[Bill Medland]

On April 15, 2003 10:12 am, Dan Kegel wrote:
> 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.
>

Sorry if I gave offence.

> > /*
> >  * namedp/server.c

snip
>
> Thanks for posting the code.

Pleasure

>  So you're checking to make sure
> that data doesn't get lost on a DisconnectNamedPipe?
>

Not really (although that might also come out in the wash).  My main aim is to ensure that until such times as FlushFileBuffers is fixed so that it blocks properly, the code I am working on doesn't crash the wine server.  I want to do so in the correct way, i.e. with a fix for an underlying problem, not a hack that needs to be removed later.

The documented way of handling the handshake according to MSDN is to use the fact that FlushFileBuffers blocks.  So what happens if someone forgot to include the FlushFIleBuffers?  They might get away with it on Windows through pure luck.  My experiment shows that it doesn't crash Windows so I think it shouldn't crash wineserver either, and actually the fix I submitted ensures that it actually returns the correct error code.

I see the FlushFileBuffers as a quite separate issue.  I am sure Mike put a load of work into what he submitted and so I am humble enough to admit that I don't know enough to fix it.

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

That's easy enough.  When flushing, if there is no complaint about the type of object, ignore any other error.
(What does that do for sock?)

Index: wine/server/fd.c
===================================================================
RCS file: /home/wine/wine/server/fd.c,v
retrieving revision 1.8
diff -u -r1.8 fd.c
--- wine/server/fd.c	26 Mar 2003 01:32:18 -0000	1.8
+++ wine/server/fd.c	15 Apr 2003 17:53:22 -0000
@@ -987,7 +987,10 @@
 
     if ((obj = get_handle_obj( process, handle, access, NULL )))
     {
-        if (!(fd = get_obj_fd( obj ))) set_error( STATUS_OBJECT_TYPE_MISMATCH);
+        fd = get_obj_fd (obj);
+        /* NB It is the object's responsibility to set the error if it
+         * has no fd
+         */
         release_object( obj );
     }
     return fd;
@@ -1003,6 +1006,8 @@
         fd->fd_ops->flush( fd );
         release_object( fd );
     }
+    else if (get_error() != STATUS_OBJECT_TYPE_MISMATCH)
+        set_error(0);
 }
 
 /* get a Unix fd to access a file */

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

Oh.

Well in that case I'll just forget it and leave the patch in place on my machine and hope that the whole thing is fixed before our company really needs it.
-- 
Bill Medland
ACCPAC International, Inc.
medbi01 at accpac.com
Corporate: www.accpac.com
Hosted Services: www.accpaconline.com