8
\$\begingroup\$

I have written the code for implementation of system() function using fork(), exec() and waitpid(). Could someone please review this code and provide feedback. Thanks a lot.


#include<stdio.h>
#include<string.h>
#include<unistd.h>
#include<sys/types.h>
#include<sys/wait.h>
#include<error.h>
#include<errno.h>
#define size 30

char *get_command(int argc, char *argv[]);
int my_system(const char *command);

int my_system(const char *command)
{
    pid_t pid;
    int wstatus  = 0;
    int ret = 0;

    if (command == NULL)
        return 1;
    pid = fork();
    if (pid == -1) {
        return -1;
    } else if (pid == 0) {
        ret = execle("/bin/sh", "sh", "-c",command, (char *)NULL);
        if (ret == -1)
            return wstatus;

    } else {
        ret = waitpid(-1, &wstatus, 0);
        if (ret == -1)
            return -1;
    }
    return wstatus;
}

char *get_command(int argc, char **argv)
{
    int i = 0;
    static char command[size];

    if (argc == 1)
        return NULL;

    strcpy(command, argv[1]);
    for (i = 2; i < argc; i++) {
        strcat(command, " ");
        strcat(command, argv[i]);
    }
    return command;
}

int main(int argc, char *argv[])
{
    int ret;
    char *command;

    command = get_command(argc, argv);
    ret = my_system(command);
    if (ret == 1)
        printf("Command is NULL, shell is available\n");
    else if (ret == -1)
        printf("Child process could not be created or error in the wait system call\n");
    else if (ret == 127)
        printf("The Child process could not be executed in the shell\n");
    else
        printf("The status of the child process is :%d\n", ret);
    return 0;
}
```
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to CodeReview. While I hope that you get good reviews, I wonder why you've tagged it as kernel. Your code is neither part of a kernel nor of a kernel module. Try to keep the tags to the essentials :) \$\endgroup\$
    – Zeta
    May 23, 2020 at 8:29

4 Answers 4

6
\$\begingroup\$

I think the biggest mistake is the return after the exec. If you reach that point, and return, you have returned to the calling function in the subprocess. Use exit() instead. Note that the value you pass to exit should come out in wstatus. Note also, if execle returns, it must have failed, so don't even both checking the status. (For this to happen, the path must be invalid. Change it for testing, and set it back afterwards.)

The use of execle() is a mistake. You didn't pass an environment. Use execl() instead.

You have at least one case where you return -1 without having set errno. Set errno. (Many cases, something else has already set errno for you.)

One other thing that can bite you: If the caller has made another subprocess, and it terminates while you are waiting for your subprocess, you will return the result of the wrong subprocess. To fix this, pass the pid as the first parameter to waitpid().

As a style issue, I would recommend moving wstatus completely into the last block. And nothing should ever come out of the compound if statement.

As a personal style point: I tend to use switch to test for -1, 0, or other. if statements work equally well.

\$\endgroup\$
3
\$\begingroup\$

In addition to the other answers, I'd add a couple of things:

size is only used in one place. You can just put the number there rather than the macro. It should also be considerably larger than 30. Consider that find /home/user/Desktop/directory is already too long, and that's not even considering long pipelines and loops.
• In addition, you should perform checking that the command entered is not too long. As it is now, you run the apparent risk of buffer overflow. Even if you protect against that by means of copying a maximum of n bytes, there is still a possible bug that can cause serious problems. If one were to enter rm -Pdrf --no-preserve-root /tmp into their terminal, it would overwrite and delete everything in /tmp as well as deleting /tmp itself. Great. But what if you only executed 30 bytes of that? You'd be left with rm -Pdrf --no-preserve-root / and a really bad day.
• As a matter of personal preference, I would separate if (pid == -1) from the chain of if - else if - else. If you return -1, you're not executing any of the other code, so it can be rewritten as

if (pid == -1)
    return -1;
if (pid == 0) //some say (!pid)
    ...
else
    ...

• Finally, I'd prefer to see command in get_command() be declared globally at the top of the file or, better, malloc()ed since you're returning the pointer anyway.

\$\endgroup\$
2
\$\begingroup\$

Here is my contribution, you are using two functions that are no safe and can create security problems. Im taking about strcpy and strcat, you should use the strncpy and strncat in order to avoid a buffer overflow on the variable commmands.

\$\endgroup\$
2
  • \$\begingroup\$ Please don't use strncat. Instead use snprintf, which is much easier to use correctly. \$\endgroup\$ May 26, 2020 at 5:35
  • 1
    \$\begingroup\$ strncpy and strncat are not for string processing, they are for array processing. Look them up in the C standard and note the careful distinction between array and string in their wording. And the footnotes. \$\endgroup\$ May 26, 2020 at 5:58
1
\$\begingroup\$

In get_command, the function name is wrong. It should be build instead of get.

You also forgot to properly escape the parameters.

./my_system printf '%n\n' 'It'\''s a nice day.'

The above call must output a single line of text.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.