From ecd994be061f4ae21f463bbf08166d8edc96cadb Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Sun, 24 Jun 2018 11:05:40 -0700 Subject: [PATCH 1/2] fixes #67 DialPipe problem with multiple calls / waiting for busy pipe This changes a few things to try to ensure that we never wind up with a result of ERROR_FILE_NOT_FOUND, due to a race between closing the last pipe instance and opening the next. First we keep an "open" client instance (unused) while the listener is open, so that we are guaranteed to always have an active pipe instance. This means attempts to open while no other instances exist result in ERROR_PIPE_BUSY instead of ERROR_FILE_NOT_FOUND. Second we have changed the loop for dialing to eliminate a race condition that is more or less inherent in WaitNamedPipe when synchronizing with CreateFile. The real timeout needs to be some larger value than the WaitNamedPipe timeout, and furthermore WaitNamedPipe is not very nice with the Go runtime, since it is a blocking system call. Instead we just put the goroutine to sleep for 10 milliseconds, and keep retrying the CreateFile until the maximum timeout is reached. If no timeout is specified we assume a reasonable and large default of 5 seconds, which is similar to a TCP connection timeout. This isn't perfect, as a client attempting to connect to an extremely busy pipe server can be starved out by other clients coming in while it is in that brief sleep, but this potential race was already present with WaitNamedPipe. The numerous retries (by default 500 retries!) mean its pretty unlikely to occur, and if a single client hits the race once, it has an excellent chance of getting in the next cycle. (A real "fix" that is completely race free and fair would require changes in the underlying Named Pipe implementation, or some other kind of external coordination.) --- pipe.go | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/pipe.go b/pipe.go index 82cbe7af..63c0fe27 100644 --- a/pipe.go +++ b/pipe.go @@ -15,7 +15,6 @@ import ( //sys connectNamedPipe(pipe syscall.Handle, o *syscall.Overlapped) (err error) = ConnectNamedPipe //sys createNamedPipe(name string, flags uint32, pipeMode uint32, maxInstances uint32, outSize uint32, inSize uint32, defaultTimeout uint32, sa *syscall.SecurityAttributes) (handle syscall.Handle, err error) [failretval==syscall.InvalidHandle] = CreateNamedPipeW //sys createFile(name string, access uint32, mode uint32, sa *syscall.SecurityAttributes, createmode uint32, attrs uint32, templatefile syscall.Handle) (handle syscall.Handle, err error) [failretval==syscall.InvalidHandle] = CreateFileW -//sys waitNamedPipe(name string, timeout uint32) (err error) = WaitNamedPipeW //sys getNamedPipeInfo(pipe syscall.Handle, flags *uint32, outSize *uint32, inSize *uint32, maxInstances *uint32) (err error) = GetNamedPipeInfo //sys getNamedPipeHandleState(pipe syscall.Handle, state *uint32, curInstances *uint32, maxCollectionCount *uint32, collectDataTimeout *uint32, userName *uint16, maxUserNameSize uint32) (err error) = GetNamedPipeHandleStateW //sys localAlloc(uFlags uint32, length uint32) (ptr uintptr) = LocalAlloc @@ -134,12 +133,14 @@ func (s pipeAddress) String() string { } // DialPipe connects to a named pipe by path, timing out if the connection -// takes longer than the specified duration. If timeout is nil, then the timeout -// is the default timeout established by the pipe server. +// takes longer than the specified duration. If timeout is nil, then we use +// a default timeout of 5 seconds. (We do not use WaitNamedPipe.) func DialPipe(path string, timeout *time.Duration) (net.Conn, error) { var absTimeout time.Time if timeout != nil { absTimeout = time.Now().Add(*timeout) + } else { + absTimeout = time.Now().Add(time.Second * 5) } var err error var h syscall.Handle @@ -148,22 +149,13 @@ func DialPipe(path string, timeout *time.Duration) (net.Conn, error) { if err != cERROR_PIPE_BUSY { break } - now := time.Now() - var ms uint32 - if absTimeout.IsZero() { - ms = cNMPWAIT_USE_DEFAULT_WAIT - } else if now.After(absTimeout) { - ms = cNMPWAIT_NOWAIT - } else { - ms = uint32(absTimeout.Sub(now).Nanoseconds() / 1000 / 1000) - } - err = waitNamedPipe(path, ms) - if err != nil { - if err == cERROR_SEM_TIMEOUT { - return nil, ErrTimeout - } - break + if time.Now().After(absTimeout) { + return nil, ErrTimeout } + + // Wait 10 msec and try again. This is a rather simplistic + // view, as we always try each 10 milliseconds. + time.Sleep(time.Millisecond * 10) } if err != nil { return nil, &os.PathError{Op: "open", Path: path, Err: err} @@ -208,6 +200,7 @@ type acceptResponse struct { type win32PipeListener struct { firstHandle syscall.Handle + clientHandle syscall.Handle path string securityDescriptor []byte config PipeConfig @@ -310,6 +303,8 @@ func (l *win32PipeListener) listenerRoutine() { } syscall.Close(l.firstHandle) l.firstHandle = 0 + syscall.Close(l.clientHandle) + l.clientHandle = 0 // Notify Close() and Accept() callers that the handle has been closed. close(l.doneCh) } @@ -354,16 +349,22 @@ func ListenPipe(path string, c *PipeConfig) (net.Listener, error) { if err != nil { return nil, err } - // Immediately open and then close a client handle so that the named pipe is - // created but not currently accepting connections. + // Create a client handle and connect it. This results in the pipe + // instance always existing, so that clients see ERROR_PIPE_BUSY + // rather than ERROR_FILE_NOT_FOUND. This ties the first instance + // up so that no other instances can be used. This would have been + // cleaner if the Win32 API matched CreateFile with ConnectNamedPipe + // instead of CreateNamedPipe. (Apparently created named pipes are + // considered to be in listening state regardless of whether any + // active calls to ConnectNamedPipe are outstanding.) h2, err := createFile(path, 0, 0, nil, syscall.OPEN_EXISTING, cSECURITY_SQOS_PRESENT|cSECURITY_ANONYMOUS, 0) if err != nil { syscall.Close(h) return nil, err } - syscall.Close(h2) l := &win32PipeListener{ firstHandle: h, + clientHandle: h2, path: path, securityDescriptor: sd, config: *c, From effecfb659e3e0fc0cbb711a9532c546fe36edae Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Wed, 18 Jul 2018 12:07:29 -0700 Subject: [PATCH 2/2] Reduce the timeout to 2 seconds, and close the client handle. We can safely close the client handle, because the server side is not closed, and thsi keeps our hold on the pipe instance. --- pipe.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/pipe.go b/pipe.go index 63c0fe27..e6c2513a 100644 --- a/pipe.go +++ b/pipe.go @@ -140,7 +140,7 @@ func DialPipe(path string, timeout *time.Duration) (net.Conn, error) { if timeout != nil { absTimeout = time.Now().Add(*timeout) } else { - absTimeout = time.Now().Add(time.Second * 5) + absTimeout = time.Now().Add(time.Second * 2) } var err error var h syscall.Handle @@ -200,7 +200,6 @@ type acceptResponse struct { type win32PipeListener struct { firstHandle syscall.Handle - clientHandle syscall.Handle path string securityDescriptor []byte config PipeConfig @@ -303,8 +302,6 @@ func (l *win32PipeListener) listenerRoutine() { } syscall.Close(l.firstHandle) l.firstHandle = 0 - syscall.Close(l.clientHandle) - l.clientHandle = 0 // Notify Close() and Accept() callers that the handle has been closed. close(l.doneCh) } @@ -362,9 +359,13 @@ func ListenPipe(path string, c *PipeConfig) (net.Listener, error) { syscall.Close(h) return nil, err } + // Close the client handle. The server side of the instance will + // still be busy, leading to ERROR_PIPE_BUSY instead of + // ERROR_NOT_FOUND, as long as we don't close the server handle, + // or disconnect the client with DisconnectNamedPipe. + syscall.Close(h2) l := &win32PipeListener{ firstHandle: h, - clientHandle: h2, path: path, securityDescriptor: sd, config: *c,