HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Send command and get response from Windows CMD prompt silently

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
responsegetcmdwindowssilentlyandcommandfromsendprompt

Problem

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("%

Solution

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 

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          // printf() and NULL
#include         // 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 primari

Code Snippets

void* g_hChildStd_IN_Rd = NULL;
void* g_hChildStd_IN_Wr = NULL;
void* g_hChildStd_OUT_Rd = NULL;
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);
// 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)
{
    ...
}
char *buf = {0};        // main()
char *Command = {0};    // cmd_rsp()
char *tmp1 = {0};       // ReadFromPipe()
char *tmp2 = {0};       // ReadFromPipe()
char *Command = {0};
//  if(!strstr(command, rqdStr))
//  {
        Command = calloc(len + sizeof(rqdStr), 1);
        strcat(Command, rqdStr);
        strcat(Command, command);
//  }

Context

StackExchange Code Review Q#162546, answer score: 5

Revisions (0)

No revisions yet.