What is wrong with my execvp usage?

Question

I'm writing a small shell to learn C. Now I want to execute custom commands but it is not working.

$ ./a.out 
OS>ls
10357: executing ls

failed to execute ls
: (2: No such file or directory)

I must not use system call to execute custom command, I should use execvp and fork. But why is it now working? The entire code is

#include<sys/stat.h>
#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <dirent.h>
#include <errno.h>
#include <stdarg.h>
#include <stdlib.h>
#include <signal.h>
int mystrcmp(char const *, char const *);

struct command
{
    char * const *argv;
};
static _Noreturn void err_syserr(char *fmt, ...)
{
    int errnum = errno;
    va_list args;
    va_start(args, fmt);
    vfprintf(stderr, fmt, args);
    va_end(args);
    if (errnum != 0)
        fprintf(stderr, "(%d: %s)\n", errnum, strerror(errnum));
    exit(EXIT_FAILURE);
}
/* Helper function that spawns processes */
static int spawn_proc(int in, int out, struct command *cmd)
{
    pid_t pid;
    if ((pid = fork()) == 0)
    {
        if (in != 0)
        {
            if (dup2(in, 0) < 0)
                err_syserr("dup2() failed on stdin for %s: ", cmd->argv[0]);
            ;
            close(in);
        }
        if (out != 1)
        {
            if (dup2(out, 1) < 0)
                err_syserr("dup2() failed on stdout for %s: ", cmd->argv[0]);
            close(out);
        }
        fprintf(stderr, "%d: executing %s\n", (int)getpid(), cmd->argv[0]);
        execvp(cmd->argv[0], cmd->argv);
        err_syserr("failed to execute %s: ", cmd->argv[0]);
    }
    else if (pid < 0)   {
        err_syserr("fork failed: ");
    }
    return pid;
}

/* Helper function that forks pipes */
static void fork_pipes(int n, struct command *cmd)
{
    int i;
    int in = 0;
    int fd[2];
    for (i = 0; i < n - 1; ++i)
    {
        pipe(fd);
        spawn_proc(in, fd[1], cmd + i);
        close(fd[1]);
        in = fd[0];
    }
    if (dup2(in, 0) < 0)    {
        err_syserr("dup2() failed on stdin for %s: ", cmd[i].argv[0]);
    }
    fprintf(stderr, "%d: executing %s\n", (int)getpid(), cmd[i].argv[0]);
    execvp(cmd[i].argv[0], cmd[i].argv);
    err_syserr("failed to execute %s: ", cmd[i].argv[0]);
}

#define BUFFERSIZE 200
int main() {

    char *args[80];
    char buffer[BUFFERSIZE];
    char *prompt = "OS";
    char *a = ">";

    char *tok;
    tok = strtok (buffer," ");


    while(buffer != NULL) {
        bzero(buffer, BUFFERSIZE);
        printf("%s%s",prompt,a);
        fgets(buffer, BUFFERSIZE, stdin);



        if(mystrcmp(buffer,"cd") == 0) {
            tok = strchr(buffer,' ')+1; //use something more powerful
            *strchr(tok, '\n')='\0';
            cd(tok);
        }
        else if(mystrcmp(buffer,"exit") == 0) {
            return 0;
        }
        else {
            //system("ls"); //for testing the CWD/PWD

            char *commandbuffer[] = { buffer, 0 };
            //char *less[] = { "less", 0 };
            struct command cmd[] = { {commandbuffer} };
            fork_pipes(1, cmd);
            printf("Spawned foreground process: %d\n", getpid());
        }
    }
    return 0;
}

int mystrcmp(char const *p, char const *q)
{
    int i = 0;
    for(i = 0; q[i]; i++)
    {
        if(p[i] != q[i])
            return -1;
    }
    return 0;
}

int cd(char *pth) {
    char path[BUFFERSIZE];
    strcpy(path,pth);

    char *token;

    char cwd[BUFFERSIZE];
    if(pth[0] != '/')
    {   // true for the dir in cwd
        getcwd(cwd,sizeof(cwd));
        strcat(cwd,"/");
        strcat(cwd,path);
        chdir(cwd);
    } else { //true for dir w.r.t. /
        chdir(pth);
    }
    printf("Spawned foreground process: %d\n", getpid());
    return 0;
}

Answer 1

The error is not with your use of execvp but with your use of fgets. fgets leaves the newline at the end of the line in the buffer, so ultimately you feed "ls\n" to execvp, and it rightly complains that it cannot find that command.

Since I'm guessing that you'll ultimately replace this code anyway, for the moment,

fgets(buffer, BUFFERSIZE, stdin);
strtok(buffer, "\n"); /* quick & dirty: remove newline if there. */

gets rid of the problem until you get around to doing the input parsing properly. I cannot recommend anything that uses strtok as a long-term solution, though. For the long term, you may be interested in the GNU-specific getline function, or indeed in libreadline (if putting your code under GPL is not a problem for you).

Answer 2

As usual, the case could be solved with strace.

Unfortunately, the code is too wrong and too long for me to write an exhaustive commentary.

meh.c:99:13: warning: implicit declaration of function 'cd' is invalid
in C99 [-Wimplicit-function-declaration]
        cd(tok);
        ^
meh.c:80:11: warning: unused variable 'args' [-Wunused-variable]
char *args[80];
      ^
meh.c:132:11: warning: unused variable 'token' [-Wunused-variable]
char *token;
      ^
3 warnings generated.

What's up with this?

#include <sys/stat.h>
#include <stdio.h>
#include <unistd.h>
#include <string.h>
#include <dirent.h>
#include <errno.h>
#include <stdarg.h>
#include <stdlib.h>
#include <signal.h>
int mystrcmp(char const *, char const *);

struct command
{
    char * const *argv;
};
static _Noreturn void err_syserr(char *fmt, ...)
{
    int errnum = errno;
    va_list args;
    va_start(args, fmt);
    vfprintf(stderr, fmt, args);
    va_end(args);
    if (errnum != 0)
        fprintf(stderr, "(%d: %s)\n", errnum, strerror(errnum));
    exit(EXIT_FAILURE);
}

Consider non-standard err func instead.

/* Helper function that spawns processes */
static int spawn_proc(int in, int out, struct command *cmd)
{
    pid_t pid;
    if ((pid = fork()) == 0)
    {
        if (in != 0)
        {
            if (dup2(in, 0) < 0)
                err_syserr("dup2() failed on stdin for %s: ", cmd->argv[0]);
            ;
            close(in);
        }
        if (out != 1)
        {
            if (dup2(out, 1) < 0)
                err_syserr("dup2() failed on stdout for %s: ", cmd->argv[0]);
            close(out);
        }

If you have to check in and out fds like this, chances are you are already doing something wrong. Consider what happens if 'out' is 0. At this level just make sure your shell always has 0,1,2 open and that will deal with the issue.

        fprintf(stderr, "%d: executing %s\n", (int)getpid(), cmd->argv[0]);
        execvp(cmd->argv[0], cmd->argv);
        err_syserr("failed to execute %s: ", cmd->argv[0]);
    }
    else if (pid < 0)   {
        err_syserr("fork failed: ");
    }

Shuffling around allows to put parent code early and avoid the indenation for long child case.

    return pid;
}

/* Helper function that forks pipes */
static void fork_pipes(int n, struct command *cmd)
{
    int i;
    int in = 0;
    int fd[2];
    for (i = 0; i < n - 1; ++i)
    {
        pipe(fd);
        spawn_proc(in, fd[1], cmd + i);
        close(fd[1]);
        in = fd[0];
    }
    if (dup2(in, 0) < 0)    {
        err_syserr("dup2() failed on stdin for %s: ", cmd[i].argv[0]);
    }
    fprintf(stderr, "%d: executing %s\n", (int)getpid(), cmd[i].argv[0]);

If printfs with the newlinewere not sufficient, strace reveals the problem:

execve("/usr/bin/ls\n", ["ls\n"], [/* 58 vars */]) = -1 ENOENT (No such file or directory)

    execvp(cmd[i].argv[0], cmd[i].argv);

You do realise this overwrites your shell?

    err_syserr("failed to execute %s: ", cmd[i].argv[0]);
}

#define BUFFERSIZE 200
int main() {

    char *args[80];
    char buffer[BUFFERSIZE];
    char *prompt = "OS";
    char *a = ">";

    char *tok;
    tok = strtok (buffer," ");


    while(buffer != NULL) {
        bzero(buffer, BUFFERSIZE);
        printf("%s%s",prompt,a);
        fgets(buffer, BUFFERSIZE, stdin);



        if(mystrcmp(buffer,"cd") == 0) {
            tok = strchr(buffer,' ')+1; //use something more powerful
            *strchr(tok, '\n')='\0';
            cd(tok);
        }
        else if(mystrcmp(buffer,"exit") == 0) {
            return 0;
        }
        else {
            //system("ls"); //for testing the CWD/PWD

            char *commandbuffer[] = { buffer, 0 };
            //char *less[] = { "less", 0 };
            struct command cmd[] = { {commandbuffer} };
            fork_pipes(1, cmd);
            printf("Spawned foreground process: %d\n", getpid());
        }
    }
    return 0;
}

int mystrcmp(char const *p, char const *q)
{
    int i = 0;

What's up with this initialisation?

    for(i = 0; q[i]; i++)

Incorrect. You assume q is not longer than p.

    {
        if(p[i] != q[i])
            return -1;
    }

There are better ways than char-by-char comparison.

    return 0;
}

What is this for anyway?

int cd(char *pth) {
    char path[BUFFERSIZE];
    strcpy(path,pth);

path and pth? Man. Consider 'orig_path' or something. Variations of one /word/ look like a typo and in fact you can easily mistype it by accident. fscking avoid.

    char *token;

    char cwd[BUFFERSIZE];
    if(pth[0] != '/')
    {   // true for the dir in cwd
        getcwd(cwd,sizeof(cwd));
        strcat(cwd,"/");
        strcat(cwd,path);
        chdir(cwd);

This is incorrect even while ignoring usual buffer overflow problems and missing error checking. If directory tree relevant to this process is modified after you getcwd, you enter the wrong directory (that's assuming chdir succeeds). What's more, paths including '..' are sensitive to symlinks.1

    } else { //true for dir w.r.t. /
        chdir(pth);
    }
    printf("Spawned foreground process: %d\n", getpid());

Seems like a copy-pasto?

    return 0;
}