Send command and get response from Windows CMD prompt silently

Question

Note - The response to reviews in this post is here.

The need:

Simply stated: _popen() for Windows

I needed a method to programmatically send commands to the Windows 7 (and newer) Command Prompt, (aka CMD console), and return the response into a buffer without seeing a console popup. The design should provide for easy implementation into c applications needing this functionality.

The design:

The development environment in addition to the Windows 7 OS is an ANSI C (C99) compiler from National Instruments, and the Microsoft Windows Driver Kit for Windows 8.1. Among the design goals was to present a very small API, including well documented and straightforward usage instructions. The result is a single exported function, with a prototype having 3 arguments. In its provided form, it is intended to be built as a DLL. The only header files I used on my environment were windows.h and stdlib.h.

For review consideration:

The code posted is complete, and I have tested it, but I am new to using pipes to stdin and stdout, as well as using Windows methods for CreateProcess(...). Also, because the size requirements of the response buffer cannot be known at compile time, the code also includes the ability to grow the response buffer as needed during run-time. For example, during testing I read directories recursively using dir /s from many locations. (excluding the root c:\ directory) For example:

cd c:\dev && dir /s  // approximately 1.8Mbyte buffer is returned on my system

I am particularly interested in having feedback focused on the following:

• Pipe creation and usage
• CreateProcess usage
• Method used for dynamically growing response buffer

Usage example:

#include "cmd_rsp.h"
int main(void)
{
    char *buf = {0};
    buf = calloc(100, 1);
    if(!buf)return 0;
    cmd_rsp("dir /s", &buf, 100);//where "dir /s" could be any command line
                                 //input that results in a stdout response
    printf("%s", buf);
    free(buf);
    
    return 0;
}

cmd_rsp.h

#ifndef _CMD_RSP_H_
#define _CMD_RSP_H_

//Exported Functions

/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//
//  Prototype:      int cmd_rsp(char *command, char **chunk, size_t size)  
//
//  Description:    Executes any command that can be executed in a Windows cmd prompt and returns
//                  the response via auto-resizing buffer.
//
//  Inputs:         const char *command - string containing complete command to be sent
//                  char **chunk - initialized pointer to char array to return results
//                  size_t size - Initial memory size in bytes char **chunk was initialized to.
//
//  Return:         0 for success
//                 -1 for failure
//
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
int __declspec(dllexport) cmd_rsp(const char *command, char **chunk, unsigned int size);
#endif //_CMD_RSP_H 

cmd_rsp.c

#include "cmd_rsp.h" 

/* child process's STDIN is the user input or data that you enter into the child process - READ */
void * g_hChildStd_IN_Rd = NULL;
/* child process's STDIN is the user input or data that you enter into the child process - WRITE */
void * g_hChildStd_IN_Wr = NULL;
/* child process's STDOUT is the program output or data that child process returns - READ */
void * g_hChildStd_OUT_Rd = NULL;
/* child process's STDOUT is the program output or data that child process returns - WRITE */
void * g_hChildStd_OUT_Wr = NULL;

// Private prototypes
int CreateChildProcess(const char *cmd);
int ReadFromPipe(char **rsp, unsigned int size);
char * ReSizeBuffer(char **str, unsigned int size);

/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
//
//  Prototype:      int cmd_rsp(char *command, char **chunk, size_t size)  
//
//  Description:    Executes any command that can be executed in a Windows cmd prompt and returns
//                  the response via auto-resizing buffer.
//
//  Inputs:         const char *command - string containing complete command to be sent
//                  char **chunk - initialized pointer to char array to return results
//                  size_t size - Initial memory size in bytes of char **chunk.
//
//  Return:         0 for success
//                 -1 for failure
//
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////
/////////////////////////////////////////////////////////////////////////////////////////////////////////////////

int __declspec(dllexport) cmd_rsp(const char *command, char **chunk, unsigned int size)
{
    SECURITY_ATTRIBUTES saAttr;
    
    /// All commands that enter here must contain (and start with) the substring: "cmd.exe /c 
    /// /////////////////////////////////////////////////////////////////////////////////////////////////////////
    /// char cmd[] = ("cmd.exe /c \"dir /s\"");  /// KEEP this comment until format used for things like
                                                 /// directory command (i.e. two parts of syntax) is captured
    /// /////////////////////////////////////////////////////////////////////////////////////////////////////////
    
    const char rqdStr[] = {"cmd.exe /c "};
    int len = (int)strlen(command);
    char *Command = {0};
//  if(!strstr(command, rqdStr))
//  {
        Command = calloc(len + sizeof(rqdStr), 1);
        strcat(Command, rqdStr);
        strcat(Command, command);
//  }


    // Set the bInheritHandle flag so pipe handles are inherited. 
    saAttr.nLength = sizeof(SECURITY_ATTRIBUTES);
    saAttr.bInheritHandle = TRUE;
    saAttr.lpSecurityDescriptor = NULL;

    //child process's STDOUT is the program output or data that child process returns
    // Create a pipe for the child process's STDOUT. 
    if (!CreatePipe(&g_hChildStd_OUT_Rd, &g_hChildStd_OUT_Wr, &saAttr, 0))
    {
        return -1;
    }

    // Ensure the read handle to the pipe for STDOUT is not inherited.
    if (!SetHandleInformation(g_hChildStd_OUT_Rd, HANDLE_FLAG_INHERIT, 0))
    {
        return -1;
    }

    //child process's STDIN is the user input or data that you enter into the child process
    // Create a pipe for the child process's STDIN. 
    if (!CreatePipe(&g_hChildStd_IN_Rd, &g_hChildStd_IN_Wr, &saAttr, 0))
    {
        return -1;
    }

    // Ensure the write handle to the pipe for STDIN is not inherited. 
    if (!SetHandleInformation(g_hChildStd_IN_Wr, HANDLE_FLAG_INHERIT, 0))
    {
        return -1;
    }
    
    //eg: CreateChildProcess("adb");
    if(CreateChildProcess(Command) == 0)
    {
        ReadFromPipe(chunk, size); 
    }
    free(Command);

    // Read from pipe that is the standard output for child process. 
    
    return 0;
}

// Create a child process that uses the previously created pipes for STDIN and STDOUT.
int CreateChildProcess(const char *cmd)
{
    //TCHAR szCmdline[] = TEXT("cmd.exe /c \"C:\\path\\to\\exe\\program.exe -parameter C:\\path\\to\\file\\file.txt\"");
    PROCESS_INFORMATION piProcInfo;
    STARTUPINFO siStartInfo;
    BOOL bSuccess = FALSE;

    // Set up members of the PROCESS_INFORMATION structure. 

    ZeroMemory(&piProcInfo, sizeof(PROCESS_INFORMATION));

    // Set up members of the STARTUPINFO structure. 
    // This structure specifies the STDIN and STDOUT handles for redirection.

    ZeroMemory(&siStartInfo, sizeof(STARTUPINFO));
    siStartInfo.cb = sizeof(STARTUPINFO);
    siStartInfo.hStdError = g_hChildStd_OUT_Wr;
    siStartInfo.hStdOutput = g_hChildStd_OUT_Wr;
    siStartInfo.hStdInput = g_hChildStd_IN_Rd;
    siStartInfo.dwFlags |= STARTF_USESTDHANDLES;

    // Create the child process. 

    bSuccess = CreateProcess(NULL,
        cmd,                // command line 
        NULL,               // process security attributes 
        NULL,               // primary thread security attributes 
        TRUE,               // handles are inherited 
        CREATE_NO_WINDOW,   // creation flags 
        NULL,               // use parent's environment 
        NULL,               // use parent's current directory 
        &siStartInfo,       // STARTUPINFO pointer 
        &piProcInfo);       // receives PROCESS_INFORMATION 

    // If an error occurs, exit the application. 
    if (!bSuccess)
    {
        return -1;
    }
    else
    {
        // Close handles to the child process and its primary thread.
        CloseHandle(piProcInfo.hProcess);
        CloseHandle(piProcInfo.hThread);
        CloseHandle(g_hChildStd_OUT_Wr);
    }
    return 0;
}

// Read output from the child process's pipe for STDOUT
// Grow the buffer as needed
// Stop when there is no more data. 
int ReadFromPipe(char **rsp, unsigned int size)
{
    COMMTIMEOUTS ct;
    int size_recv = 0;
    unsigned int total_size = 0;
    unsigned long dwRead;
    BOOL bSuccess = TRUE;
    char *accum;
    char *tmp1 = {0};
    char *tmp2 = {0};
    
    
    //Set timeouts for stream
    ct.ReadIntervalTimeout = 0;
    ct.ReadTotalTimeoutMultiplier = 0;
    ct.ReadTotalTimeoutConstant = 10;
    ct.WriteTotalTimeoutConstant = 0;
    ct.WriteTotalTimeoutMultiplier = 0;
    SetCommTimeouts(g_hChildStd_OUT_Rd, &ct);

                                                          
    //This accumulates each read into one buffer, 
    //and copies back into rsp before leaving
    accum = (char *)calloc(1, sizeof(char)); //grow buf as needed
    if(!accum) return -1;
    memset(*rsp, 0, size);
    
    do
    {
        //Reads stream from child stdout 
        bSuccess = ReadFile(g_hChildStd_OUT_Rd, *rsp, size-1, &dwRead, NULL);
        if (!bSuccess || dwRead == 0) 
        {
            free(accum);
            return 0;//successful - reading is done
        }
        
        (*rsp)[dwRead] = 0;
        size_recv = (int)strlen(*rsp);


        if(size_recv == 0)
        {
            //should not get here for streaming
            (*rsp)[total_size]=0;
            return total_size;
        }
        else
        {
            //New Chunk:
            (*rsp)[size_recv]=0;
            //capture increased byte count
            total_size += size_recv+1;
            //increase size of accumulator
            tmp1 = ReSizeBuffer(&accum, total_size);
            if(!tmp1)
            {
                free(accum);
                strcpy(*rsp, "");
                return -1;
            }
            accum = tmp1;
            strcat(accum, *rsp);
            if(total_size > (size - 1))
            {   //need to grow buffer
                tmp2 = ReSizeBuffer(&(*rsp), total_size+1);
                if(!tmp2)
                {
                    free(*rsp);
                    return -1;
                }
                *rsp = tmp2;
            }
            strcpy(*rsp, accum);//refresh rsp
        }
        
        
    }while(1);
}

// return '*str' after number of bytes realloc'ed to 'size'
char * ReSizeBuffer(char **str, unsigned int size)
{
    char *tmp={0};

    if(!(*str)) return NULL;

    if(size == 0)
    {
        free(*str);
        return NULL;
    }

    tmp = (char *)realloc((char *)(*str), size);
    if(!tmp)
    {
        free(*str);
        return NULL;
    }
    *str = tmp;

    return *str;
}

Answer 1

The question is very interesting, and obviously required some real time to implement.

Just a few observations and suggestions:

In the declaration of cmd_rsp() the variable size might be better named chunk_size or ChunkSize to indicate it is the buffer size. The function cmd_rsp() might be altered to return the output string or NULL in case of error, this removes the need to pass in the buffer and any possible buffer overflows.

Global Symbols

While the cmd_rsp.h file does not include the declarations of these variables and functions, the scope of these variables and functions is global and may impact names within the program itself. It is also possible that a programmer trying to maintain the code will add the variables or functions in another part of the program, this can either lead to link time problems or bugs through unintentional side affects.

If the goal of cmd_rsp.h and cmd_rsp.c is to eventually become either a static library or a dynamically linked library this is problematic practice.

Global Variables

While these variables could be declared static like the functions below a better approach might be to declare them within the function cmd_rsp(const char *command, char **chunk, unsigned int size). Not only does this remove the variables from the program name space, it also makes the code easier to write, maintain and debug. The file cmd_rsp.c is already 254 lines long, if features are added in the future by you or someone else it becomes very difficult to find where side affects may change the values of these variables.

void* g_hChildStd_IN_Rd = NULL;
void* g_hChildStd_IN_Wr = NULL;
void* g_hChildStd_OUT_Rd = NULL;
void* g_hChildStd_OUT_Wr = NULL;

Global Functions

In the cmd_rsp.c file there is the following code with comments:

// Private prototypes
int CreateChildProcess(const char *cmd);
int ReadFromPipe(char **rsp, unsigned int size);
char * ReSizeBuffer(char **str, unsigned int size);

The way the function prototypes are defined, they are global to any progam that links in cmd_rsp.c. To truely make the functions private use the static keyword in both the prototype and the function declaration.

// Private prototypes
static int CreateChildProcess(const char *cmd);
static int ReadFromPipe(char **rsp, unsigned int size);
static char * ReSizeBuffer(char **str, unsigned int size);

static int CreateChildProcess(const char *cmd)
{
    ...
}

Improper Initialization of Pointers

The following declarations are allocating arrays of characters of size 1. Is this what was desired?

char *buf = {0};        // main()
char *Command = {0};    // cmd_rsp()
char *tmp1 = {0};       // ReadFromPipe()
char *tmp2 = {0};       // ReadFromPipe()

So the following code is throwing away a character array of size 1: (possible memory leak)

    char *Command = {0};
//  if(!strstr(command, rqdStr))
//  {
        Command = calloc(len + sizeof(rqdStr), 1);
        strcat(Command, rqdStr);
        strcat(Command, command);
//  }

There are 2 more problems with the previous code, one is specific to the CodeReview.StackExchange.com website:

• Command is not tested to see if the calloc() actually returned memory or if there was a memory allocation error.
• Quite often on CodeReview commented out code indicates bugs and the question will be flagged as broken code. If the strstr() test is to be removed, then it shouldn't be in the question.

If the desired goal was to initialize the pointers to a null value, then the proper way to do this is to include one of the following files and use the NULL macro, stddef.h, stdlib.h, or stdio.h. The second answer to this stackoverflow question provides a larger list of files that supply NULL.

#include <stdlib.h>

char *buf = NULL;
char *Command = NULL;

If the code isn't going to include the header files for some reason see this stackoverflow question for better ways to define NULL.

The Example Usage Should Provide Proper Usage

The example doesn't test the results of the call to cmd_rsp(). A better example might be:

#include <stdio.h>         // printf() and NULL
#include <stdlib.h>        // EXIT_FAILURE, EXIT_SUCCESS and NULL
#include "cmd_rsp.h"

#define MY_BUFFER_SIZE    100

int main(void)
{
    int status = EXIT_SUCCESS;
    char *buf = NULL;
    buf = calloc(MY_BUFFER_SIZE, 1);
    if(!buf)return 0;
    if (!cmd_rsp("dir /s", &buf, MY_BUFFER_SIZE))
    {
        printf("%s", buf);
    }
    else
    {
        status = EXIT_FAILURE;
    }
    free(buf);

    return status;
}

Reduce Complexity, Follow SRP

The Single Responsibility Principle states that every module or class should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated by the class. All its services should be narrowly aligned with that responsibility.

Robert C. Martin expresses the principle as follows:

    A class should have only one reason to change.

While this is primarily targeted at classes in object oriented languages it applies to functions and subroutines in procedural languages like C as well.

The function cmd_rsp(const char *command, char **chunk, unsigned int size) is overly complex and could be broken up into at least 2 sub-functions:

void SetupSecurityAttributes(SECURITY_ATTRIBUTES *SECURITY_ATTRIBUTES saAttr);
int SetupChildIOPipes(void* g_hChildStd_OUT_Rd, void* g_hChildStd_OUT_Wr, void* g_hChildStd_IN_Rd, void* g_hChildStd_IN_Wr, SECURITY_ATTRIBUTES* saAttr);

The function int ReadFromPipe(char **rsp, unsigned int size) is too complex as well, the contents of the do { } while loop in particular could be at least one additional function called from ReadFromPipe().

It might also be beneficial to reduce the repetition of the code in cmd_rsp() if possible. See Don't Repeat Yourself.