Spotting Code Defects - #1 (Named Pipe Server)

When you read bad code you learn what not to do.  You learn to identify the many classes of errors and the patterns that often lead up to them.

 

When you read good code you learn how to write good code.  You observe “tricks” of good programmers (the best tricks aren’t tricks at all), you see defensive coding, pessimistic coding, efficient coding.  Robust code that can run anywhere at any time and handle any situation with grace and security.

 

The question is – do you know the difference between good and bad code?

 

This is a basic pipe server.  It sits around waiting for a client to connect, reads some input and performs some action (the action is not implemented, it’s not a meaningful detail), and when it’s done it write backs to the client to acknowledge that it performed the action. 

 

To ensure that the client does not do something they don’t have permissions to do, the server impersonates the client before performing that magical action (so assume it could be registry read/write, executing a process, processing some file, etc).

 

While this is a console application, assume that this could be running under a privileged account on the system (e.g. member of Local Administrators).

 

I wish that .Text allowed me to hide the comments until I post the defects, but they don’t.  So if you don’t want spoilers don’t read the comments.  If you do … then do.  If someone else has already posted a defect feel free to list it again (if you really identified it on your own).  It’s the honor system.

 

The only prize is a warm fuzzy feeling you get from participating.

 


 

#include <tchar.h>

#include <windows.h>

#include <stdio.h>

int _tmain(int argc, TCHAR **argv)

{

      if(argc != 2)

      {

            _tprintf(_T("pipeServer.exe <pipeName>\n"));

            return 100;

      }

      TCHAR lpszPipeName[256] = _T("\\\\.\\pipe\\");

      _tcscat(lpszPipeName, argv[1]);

      HANDLE hPipe = CreateNamedPipe(

            lpszPipeName,

            PIPE_ACCESS_DUPLEX,

            0,

            PIPE_UNLIMITED_INSTANCES,

            512,

            512,

            INFINITE,

            NULL);

      if(hPipe != NULL)

      {

            for(;;)

            {

                  if(ConnectNamedPipe(hPipe, NULL))

                  {

                        TCHAR lpszCommand[1024];

                        DWORD dwRead = 0;

                        ImpersonateNamedPipeClient(hPipe);

                       

                        for(;;)

                        {

                              if(ReadFile(hPipe, lpszCommand, sizeof(lpszCommand), &dwRead, NULL))

                              {

                                    if(_tcscmp(lpszCommand, _T("QUIT")) == 0)

                                    {

                                          break;

                                    }

                                    else

                                    {

                                          /* PERFORM MAGICAL ACTION based on user input */

                                          DWORD dwWrote = 0;

                        WriteFile(hPipe, lpszCommand, dwRead, &dwWrote, NULL);

                                    }

                              }

                        }

                        RevertToSelf();

                        DisconnectNamedPipe(hPipe);

                  }

                  else

                  {

                        CloseHandle(hPipe);

                        break;

                  }

            }

      }

}