execv() system call creating stacking smashing error

Question

Whenever I used the execv() here in my code, it works and has no errors, but still causes stack smashing to crash the program during runtime. Am I doing anything wrong here? Here is the function with the execv():

void execute(char *args[], char *cmd[])
{
    pid_t pid;
    int status;
    char bin[10] = "/bin/";
    pid = fork();

    // child process
    if(pid == 0)
    {
         strcat(bin, cmd[0]);

    execv(bin, args);   
    } else{
   perror("error");
    while(wait(&status) != pid);
    }
}

here is where I am getting args and cmd from. Would it possibly be caused by something I did here?

void parseString(char *command)
{
    char **args = malloc(sizeof(char*) * 16);
    int i = 0;
    int j = 0;
    char *cmd[1];

    // split each command by semicolons if necessary, then send each sub        command to parseString()
    if(strchr(command, ';')) {
        char *semi_token = strtok(command, ";");
        while(semi_token != NULL){
            args[i] = semi_token;
            semi_token = strtok(NULL, " ");
            parseString(args[i]);
            i++;
        }
    } else {
        // if no semi colons, split the commandby spaces and call execute() using the args and cmd
        char *token = strtok(command, " ");
        while(token != NULL)
        {
            args[i] = token;
            args[++i] = NULL;
            while(j == 0 && token != NULL) {
                    cmd[0] = token;
                    cmd[1] = NULL;
                    j++;
                }
            token = strtok(NULL, " ");
            }
            execute(args, cmd);
        }

        j = 0;
        i = 0;
        free(args);
    }

function call happens here. command is input from stdin from the user. Only need basic commands all located in /bin/. something like ls -l or cat file.

while(1){
        command = getCommand();
        parseString(command);
}

In your `execute` function, what is the contents of `cmd[0]`? Will it ever be longer than four characters? Because that's all that can fit in `bin`. — Some programmer dude, Mar 18 '18 at 02:35
And you are limiting to 15 arguments only, do you have more than 15 arguments? — Pablo, Mar 18 '18 at 02:40
Always calling `perror("error");` in the parent is wrong. Arguably, you should detect `pid == -1` (or `pid < 0`) and then reporting an error would be appropriate. You've not shown how you call your code, nor explained what you're providing as input. You've generally not created an MCVE ([MCVE]), so it is difficult to see what you're up to. But you've been excessively parsimonious in your `execute()` function — `char bin[100] = "/bin/";` might be better, but not every command is in `/bin` anyway (there's `/usr/local/bin`, and sometimes `/usr/bin` is separate from `/bin`). — Jonathan Leffler, Mar 18 '18 at 02:40
By the way, don't forget that `execv` can fail as well. And with your current program that can happen more than you think, and lead to bad things. — Some programmer dude, Mar 18 '18 at 02:44
Or put an `exit(1)` after the `execv` so that the child process exits after a failure of `execv`. — Pablo, Mar 18 '18 at 02:47
exit(1) didn't exit out of the child process. The process completes and execv() completes successfully. Where the program seems to stop working is at the wait(). Also added function calls and what would be inputted. — John Novosel, Mar 18 '18 at 02:51

Some programmer dude · Answer 1 · 2018-03-18T04:12:46.040

You have two serious errors: One that will lead to out-of-bounds writing of an array, and one that will probably lead to that.

The first, the certain out-of-bounds writing, is in the parseString function. First you have the declaration of the cmd variable:

char *cmd[1];

This defines cmd as an array of one element. Then you do

cmd[0] = token;
cmd[1] = NULL;

which writes to two elements of the one-element array. Writing out of bounds leads to undefined behavior.

The second error is in the execute function, and is the one I talked about in my first comment. There you have

char bin[10] = "/bin/";

That defines bin as an array of ten characters, and you fill up six of them (don't forget the string terminator). In the child-process you do

strcat(bin, cmd[0]);

which appends to string in cmd[0] to the string in bin. The problem here is that bin only have space for ten characters, of which six is already used (as explained above). That means there's only space left for four characters. If the command is any longer than that you will also go out of bounds and again have undefined behavior.

The solution to the first error is simply, make cmd an array of two elements. The solution to the second error is to either make bin larger, and not concatenate more than can fit in the array; Or to allocate the array dynamically (not forgetting space for the terminator).

There are also lot of other potential problems with your code, like the limit on 16 pointers for args. And that you don't really parse arguments in the parseString function, every argument is seen as a separate command. And the memory leak in the case there are semicolon-separated "commands". Or that you don't check for or handle errors everywhere needed. And that you use errno even if there's no error.

execv() system call creating stacking smashing error

1 Answers1