TL;DR: Solved – after several failed attempts I discovered how to detect client disconnects when using named pipes in C# — the article below includes relevant code snippets from my CSNamedPipes GitHub project.
In my spare time — of which there’s not much since I have four kids — I’ve been helping someone solve a programming challenge related to inter-process communication between a desktop application and a Windows service application. The desktop app makes requests to the service app to perform tasks which require administrative permissions, like performing content updates for files that are stored in privileged folders.
Communicating between two applications seems like it ought to be straightforward, but like any programming task there is always undiscovered territory. Since the two applications are running in different window stations, on different desktops, and in different security contexts, the best way for the apps to talk is to use named pipes. Simple, right?
First challenge: the code needed to be written in C#. While I’ve read C# code, and even debugged C# code written by others, I’ve never actually written anything in C# myself. I figured I’d just Google some code and bang it into shape, because this seems like a problem that many folks must have solved before me. I wanted something that used asynchronous APIs (more optimal than synchronous communication — something I know from writing lots of server code) and handled all the common error cases; that’s not too much to ask, right?!? Sadly, after reading through a great deal of code found online I didn’t discover anything that fit the bill. The best was from Jeffrey Richter, who writes great code and books. I started with his code sample (see Ch28-1-IOOps.cs in the ZIP file) as a base.
But I quickly ran into a problem that seems to be common to all the implementations I found: the named pipe server has trouble detecting when a client disconnects. This is a big problem because it can be used by a malicious client to cause a denial-of-service attack.
My thought was that the C# library should detect that the pipe closed and automatically perform a callback with a zero-byte read, but this turns out not to be the case.
The basic pattern for an asynchronous read is this:
{
...
m_pipe.BeginRead(m_data, 0, data.Length, OnAsyncMessage, null);
}
private void OnAsyncMessage(IAsyncResult result) {
Int32 bytesRead = m_pipe.EndRead(result);
if (bytesRead != 0) {
// good times -- process the message
}
else
// pipe disconnected, right?!? NOPE!
}
}
Google for “How to detect a client disconnect using a named pipe” and you’ll get 430000 hits, and even more if you play with variations of the search! So clearly I’m not the only one having trouble…
It occurred to me to set a read timeout on the PipeStream object, but that doesn’t work: async pipes don’t support that feature.
I could set a timer to periodically cancel the read and start a new one, but what happens if I cancel the read on one thread while it being started in another thread? That seemed like a disaster waiting to happen.
Another idea was to have the server “ping” the client periodically to ensure the connection is still alive, but that’s lame because I’m writing a low-level network library. If I have to send pings I either have to define the application-level network protocol so that I can send pings, or I have to put the burden of pinging on the application layer code, which just places a burden on folks using my API.
So I ended up adding a background thread that polls to discover whether each connection is still alive. This is hacky: if C# already knows the connections have been closed, why isn’t it bothering to tell me?!? Update: it’s possible to detect disconnects without polling; I’ve updated the code in the github archive based on a suggestion by Chae Seong Lim, someone I worked with for many years at ArenaNet. Thanks CS!
An important point to note when writing your own IPC code using named pipes: when setting the pipe security permissions, you must include FullControl permission for the current user, otherwise creating the second instance of the pipe will fail (thanks Chris Dickson).
m_ps = new PipeSecurity();
m_ps.AddAccessRule(new PipeAccessRule(
WindowsIdentity.GetCurrent().User,
PipeAccessRights.FullControl,
AccessControlType.Allow));
m_ps.AddAccessRule(new PipeAccessRule(
new SecurityIdentifier(WellKnownSidType.AuthenticatedUserSid, null),
PipeAccessRights.ReadWrite, AccessControlType.Allow));
As a bonus, here’s the complete code for the project that implements the solution on my GitHub page CSNamedPipes.
About Patrick Wyatt
lock(this) may have unintended consequences if some outside code can access your object ant try locking on it. Probably not an issue here but worth mentioning :)
Omeg, you’re right that any user application code which calls my API and has access to an IpcServer object can cause nasty problems — like a deadlock — in the event the app decided to lock the object.
To prevent the user application code from doing evil I could hide away the IpcServer object in a static variable (a singleton) that’s not exposed publicly, and change the IpcServer API from object-based to procedural-based. But that solution has it’s own flaw — what if I want to have several different types of named pipes open for different types of clients? I would then have to add a private hash-table of IpcServer objects referenced by something (say, a token or string), but that would add some programming cruft.
It is critical protect APIs that are externally visible to other applications (that is, network APIs) from malicious behavior. In this instance the IpcServer API is an internal to the application. If the application writer wants to lock the IpcServer object, he’s misguided, but it’s probably better to let him than jump over too many hurdles to try and stop that behavior. Locking random objects is not recommended.
Thanks for your comment; emphasizing good defensive programming is something we should all strive for!
A common solution to the locking question is to have a private read-only instance field of Object type, and lock on that. This keeps the whole synchronization context private to the class.
An alternate approach would be to lock on the dictionary, if it never escapes the class. This saves you from a junk field, but that mainly comes down to aesthetics and local conventions.
Thank you; I’ve chosen to lock on the dictionary as you suggested.
I don’t think you needed to mention that you don’t write a lot of C#. As far as standard practice, you should be using a timer rather than a timeout on a wait handle, locking a private object (rather than “this”), looping over the Keys property of the dictionary (since you never touch the values), and probably using a LINQ where to make your check more idiomatic (i.e. foreach ( var pipe in m_pipes.Keys.Where( p => !p.IsConnected ) ) pipe.Close();).
It’s disappointing that some folks feel the need to behave badly when they’re on the Internet. I always wonder what motivates people to act out like that, and whether such negative behavior ultimately makes the posters feel any better about themselves. However, the post by AnonymousA is interesting in that it contains such useful advice amid the hostility that I’ve chosen to keep it.
I’ve incorporated two of the suggested changes into the code and will consider updates for the others when I have more time.
Wouldn’t it be awesome if AnonymousA wrote a blog and shared these techniques in more constructive fashion? Of course, it would expose him/her to similar criticisms from others, which might imbue a greater sense of humility. And we would all have the opportunity to learn from those posts, as I hope that some of you have been able to do from my article above.
Obviously 3 months after the fact, but I see this kind of passive-aggressive behavior from a lot of programmers on the internet (at least those who choose to remain anonymous). I’m really glad you called him out on it, it seems pretty common for such people when they stumble upon someone with less knowledge in a given field to throw in condescension with their “advice”.
A shame people can’t all just act respectfully and share knowledge enthusiastically.
Very nice — this will save me some hair pulling.
It would be nice to enable Task on your IPCLib, make it cancel-able, etc. (http://stackoverflow.com/questions/10134310/how-to-cancel-a-task-in-await)
If I figure that out, I’ll post you back a message.
Thanks!
Patrick, just wanted to say thanks for this great little library. It helped me get IPC up and running between several apps in no time.
Hi, I created a IpcServer and IpcClient with different approach which uses Reactive Extensions. Where my IpcServer checks for client disconnection is derived from your code.
https://gist.github.com/hanishi/7139122
Thanks for sending along the code! I haven’t used RX before, so I’m playing with your code now to learn more about it. I hope other readers will find it useful too.
Your code taught me how I need to complete my IpcServer. Especially the part where NamedPipeServerStream is recursively generated. Thank you very much!
So what could have happened when a read ends with 0 bytes?
I wrote that code a while back and should have commented it! Nothing like a year long gap to forget…
In any event, when using sockets, a zero byte read indicates that the socket has been closed, but with named pipes that is (apparently) not the case, so I added the guard.
Thank you for your work on this. Your code helped me understand the process and I was able to build my own ipc service based on what you did. As a quick note for someone in the future working with your code. The max read is 4k. While the block size can be adjusted larger, it’s probably better to read in 4k blocks until complete. This code will allow you to do that in the ipcclient.cs file:
string decoded = string.empty;
do {
bytesRead = pipe.Read(data, 0, data.Length);
decoded += Encoding.UTF8.GetString(data, 0, bytesRead);
} while (pipe.IsMessageComplete == false);
thanks again! You don’t know how much trouble this subject was giving me until I found your blog and source.
Glad you found the code helpful!
You’re right; I hard-coded the buffer size to be relatively small at only 4K, which might not be appropriate for your application. However, the code you’ve written is dangerous in that it leaves your server vulnerable to a denial-of-service attack, where a client can cause your server to run out of memory. It would be preferable to handle data-aggregation at the application layer (“your code”) instead of the network layer (my code in IpcServer class).
How you’d handle this is that the first few bytes of a new message would contain “framing information”: what type of message this is, and what the total size should be (and perhaps a checksum). Then you can evaluate the source of the message (which user sent it), the connection state (has this user consumed too much of his/her quota), how big the message is relative to the request (e.g. “ping” messages shouldn’t be 64GB long), & etc. That also gives you the opportunity to pre-allocate the correct buffer size, which avoids the memory-garbage creating code that “decoding +=” creates, which chews through memory like nobody’s business.
Ok, maybe things got changed in the meantime, but according to MSDN EndRead() returns: “The number of bytes that were read. A return value of 0 indicates the end of the stream (the pipe has been closed).” That’s how the documentation for .NET 3.5 looks as well, so either the MSDN documentation is wrong (totally plausible) or I’m missing something?
You’re correct that EndRead returning zero bytes signals the pipe is closed. While I don’t handle that in the IpcServer::EndRead, it is detected in IpcServer::BeginRead, which discovers that state immediately before reading more data. It’s preferable to have only a single code-path that handles socket/pipe/file/thing termination as that reduces bugs, so I handle both cases (e.g. “pipe disconnected before read” and “pipe disconnected after read”) in the same place.
Within your `IpcServer` class just add:
private void OnAsyncMessage(IAsyncResult result) {
try {
pd.pipe.BeginWaitForConnection(..
Ignore InvalidOperationException and catch IOException, problem solved.
Won’t have to test `bytesRead != 0`, further, `IpcServer.BeginRead` can be simplified.