using system() with command line arguments in C

Question

I am trying to make a program in Linux in which the command to be executed is passed as a command line argument. I have made a code but it runs file sometimes and sometimes it does not work and i don't know what the problem is.

Here is my code:

#include<stdio.h>
#include<string.h>
#include<unistd.h>

void main(int argc, char* argv[]){

    char com[60];
    int i;

    for(i = 1 ; i < argc ; i++){

        strcat(com, argv[i]);
        strcat(com, " ");
    }

    system(com);
}

Zeroth things zeroth: [`main` should return `int`](https://stackoverflow.com/questions/204476/what-should-main-return-in-c-and-c). — melpomene, Oct 21 '18 at 11:10
You already have all the arguments as separate strings, so you shouldn't be using `system` at all! Use `exec` instead, possibly in conjunction with `fork`. — Daniel Pryden, Oct 21 '18 at 11:13
@LakshyaMunjal Did you follow the link I posted and read the answers? — melpomene, Oct 21 '18 at 11:21
@DanielPryden could you just post the code. I have confusion in fork() and exec() functions. — Lakshya Munjal, Oct 21 '18 at 11:31
@LakshyaMunjal: That can't be answered in a comment. Can you ask a new question about the correct way to launch a child process from C code on Linux using `fork` and `exec`? — Daniel Pryden, Oct 21 '18 at 11:36
@DanielPryden: explain me how to launch child process correctly using fork and exec . — Lakshya Munjal, Oct 21 '18 at 11:41
@LakshaMunjal: When I said "ask a new question", I meant by clicking the "Ask a Question" link and opening a new question. I probably won't answer it myself, but someone else will. — Daniel Pryden, Oct 21 '18 at 11:45
Since all you need to do is to `exec` the command once (and probably do not care about the exit value), *just use* `execve` with `&argv[1]` as the pointer to the first argument in the list. Simple as that — Antti Haapala -- Слава Україні, Oct 21 '18 at 13:42

score 3 · Answer 1 · answered Oct 21 '18 at 11:12

Your program has undefined behavior.

If the user passes no arguments, the loop is never entered and you do

char com[60];
// ...
system(com);

The contents of com are uninitialized at this point, so the call to system has undefined behavior.

If the user passes arguments, you do

strcat(com, argv[i]);

The contents of com are uninitialized at this point, so the call to strcat has undefined behavior.

To fix this, make com a valid string before the loop:

char com[60];
com[0] = '\0';

Petr Skocik · Answer 2 · 2018-10-21T11:43:25.030

That'll sometimes overflow the buffer (very bad), sometimes it'll get the quoting wrong, and with no arguments, the buffer will be uninitialized (=> undefined behavior).

When a shell runs your program, it'll de-quote strings and interpolate $-variables. To do this forwarding robustly, you'd need to requote, in addition to checking against buffer overflow. That would be a bit tedious for in a short program.

The easier thing would be to simply posix_spawn or fork/exec with the argv array directly:

int status;
pid_t pid;

if (0>(pid=fork())) return -1;
if(0==pid){
    execvp(argv[1],argv+1);
    _exit(127);
}
while(0>(waitpid(pid,&status,0)))
    if (EINTR==errno) continue; else abort(); /*shouldn't happen*/

Now, a real system() would additionally ignore SIGINT/SIGQUIT in the parent and block SIGCHLD in the parent before the status is reaped. posix_spawnp would also be preferred.

To copy that right, you can start off with the musl libc's implementation of system, and modify it to create a

int my_system(char const *file, char *argv[i]);

that'll skip the shell (and with it, the need to create a string with correct quoting and buffer overflow checking):

#include <unistd.h>
#include <stdlib.h>
#include <signal.h>
#include <sys/wait.h>
#include <spawn.h>
#include <errno.h>
#include <pthread.h>

//extern char **__environ;
extern char **environ;

int my_system(char const *file, char *argv[])
{
    pid_t pid;
    sigset_t old, reset;
    struct sigaction sa = { .sa_handler = SIG_IGN }, oldint, oldquit;
    int status = -1, ret;
    posix_spawnattr_t attr;

    //pthread_testcancel();

    //if (!cmd) return 1;

    sigaction(SIGINT, &sa, &oldint);
    sigaction(SIGQUIT, &sa, &oldquit);
    sigaddset(&sa.sa_mask, SIGCHLD);
    sigprocmask(SIG_BLOCK, &sa.sa_mask, &old);

    sigemptyset(&reset);
    if (oldint.sa_handler != SIG_IGN) sigaddset(&reset, SIGINT);
    if (oldquit.sa_handler != SIG_IGN) sigaddset(&reset, SIGQUIT);
    posix_spawnattr_init(&attr);
    posix_spawnattr_setsigmask(&attr, &old);
    posix_spawnattr_setsigdefault(&attr, &reset);
    posix_spawnattr_setflags(&attr, POSIX_SPAWN_SETSIGDEF|POSIX_SPAWN_SETSIGMASK);

#if 0
    ret = posix_spawn(&pid, "/bin/sh", 0, &attr,
        (char *[]){"sh", "-c", (char *)cmd, 0}, environ);
#else
    ret = posix_spawnp(&pid, file, 0, &attr, argv, environ);
#endif

    posix_spawnattr_destroy(&attr);

    if (!ret) while (waitpid(pid, &status, 0)<0 && errno == EINTR);
    sigaction(SIGINT, &oldint, NULL);
    sigaction(SIGQUIT, &oldquit, NULL);
    sigprocmask(SIG_SETMASK, &old, NULL);

    if (ret) errno = ret;
    return status;
}

Now with my_system, you have no need for quoting and you can simply call it with

my_system(argv[1], argv+1);

using system() with command line arguments in C

2 Answers2

Linked