Assistance with getting the pipe in my c shell working

Question

Hi I need some assistance with my pipe method I created for my c shell. I am trying to get it to run a simple ls | wc.

I have written a method called createPipe which forks two child processes, because the standard output of cmd1 becomes the standard input of cmd2.

I have also written a method called execute() which is supposed to split the paths we have to maintain a constant path variable) and to execv the path and the commands, I have to use execv, instead of execvp or execlp etc.

When I execute my program in the part of createPipe where it hits execute(command), I keep getting that ls | wc is an invalid command.

If I attempt to just execv the commands in the createPipe method instead of using the execute method, It hits an error I inserted into my method, so I get:

no exec child1: bad address 

I am not sure what is wrong. If something is incorrect in my createPipe method, or if something is wrong in my execute method, or I am not handling things correctly in my main program. Another opinion would really help.

Here is my code:

/* main.c */

#define NUM_ARGS 10
#define BUF_SIZE 128

void utility(char *array[]);

void cleararray(char *array[])
{
    int i=0;
    while(array[i]!=NULL)
    {
        array[i]=NULL;
        i++;
    }
}

int main(void){

    int redir_Flag=0;
    int i=0,j;
    char str[BUF_SIZE];
    char *p;
    char *array[NUM_ARGS];

    do
    {
        cleararray(array);
        printf("~MyShell $:");
        i=0;
        fgets(str,sizeof(str),stdin);
        p=strtok(str," \n");
        while(p!=NULL)
        {
            array[i]=(char*)malloc(sizeof(p));
            strcpy(array[i],p); 
            p=strtok (NULL," \n");
            i++;
            array[i]=NULL;
        }

        for(j=0;j<i;j++){
            if(strcmp(array[j],">")==0)
            {
                array[j]=NULL;
                redirect(array,array[j+1]);
                redir_Flag=1;
                break;
            }
            //printf("%s %d\n",array[j],j);
       }
       //printf("Before flag checking :%s\n",array[0]);
       /////////////////////////////////////////////  beginning of pipe check
       int q;
       for(q=0; q < i; q++){
           if(strcmp(array[q], "|") == 0)
           {
               printf("%s %s\n", array[q-1], array[q+1]);
               //char **cmda=NULL;
               //char **cmdb=NULL;
               createPipe(array[q-1], array[q+1]);
           }
       }
       if(redir_Flag==1)
       {
           redir_Flag=0;
           continue;
       }
       else{}
       if(array[0]==NULL){continue;}
       else if(strcmp(array[0],"exit")==0)
       {
           break;
       }
       else if(strcmp(array[0],"cd")==0)
       {
           change_Dir(array[1]);
       }
       else if(strcmp(array[0],"path")==0)
       {
           if(array[1]==NULL)
           {
               print_path();
           }
           else if(strcmp(array[1],"+")==0)
           {
               add_path(array[2]);
           }
           else if(strcmp(array[1],"-")==0)
           {
               remove_path(array[2]);
           }
           else {printf("invalid command\n");}
       }
       else if(array[0]==NULL){continue;}
       else
       {
           utility(array);
       }
       i=0;
       fflush(stdout);
       cleararray(array);
   }while(1);
   return 0;
}

Pipe.c

void createPipe(char **cmd1, char **cmd2){

    pid_t pid1; 
    pid_t pid2;

    int newPipefd[2];

    if(pipe(newPipefd)== -1){
        perror("pipe error");
        exit(1);
    }
    pid1 = fork();
    if(pid1 == -1){
        perror("fork error");
    }
    else if(pid1==0){
        //first child process closes the read end dup write
        close(1);
        dup(newPipefd[1]);
        close(newPipefd[1]);
        close(newPipefd[0]);

        execute(cmd1);     ////////////////////////here is where I have issues
        execv(cmd1[0], cmd1);

        perror("no exec child1");
    }
    else{
        pid2 = fork();
        if(pid2 ==-1){
            perror("fork error");
    }
    else if (pid2 == 0){
        //second child process closes up write side of pipe and dup read
        close(0);
        dup(newPipefd[0]);
        close(newPipefd[0]);
        close(newPipefd[1]);

        execute(cmd2);
        execv(cmd2[0], cmd2);
    } else {
        //parent
        int status;
        close(newPipefd[0]);
        close(newPipefd[1]);
        waitpid(pid1, &status, WUNTRACED | WCONTINUED);
        waitpid(pid2, &status, WUNTRACED | WCONTINUED);
    }
}  

execute method

void execute(char *arg[]){
    int j=0,k;
    char *p;
    char *array[NUM_ENV];
    if(access(arg[0],F_OK)==0)
    {
        execv(arg[0],arg);
    }
    else
    {
        p=strtok(path,":\n");
        while(p!=NULL)
        {
            array[j]=(char*)malloc(sizeof(p));
            strcpy(array[j],p);
            p=strtok (NULL,":\n");
            printf("%s\n",array[j]);
            j++;
            array[j]=NULL;
        }
        for(k=0;k<j;k++){
            printf("in for loop : %s\n",arg[k]);
            strcat(array[k],"/");
            strcat(array[k],arg[0]);
            printf("IN FOR LOOP, ARRAY IS :%s\n",array[k]);

            if(access(array[k],F_OK)==0)
            {
                execv(array[k],arg);
            }
        }
        printf("Invalid command\n");
        fflush(stdout);
    }
}

Answer 1

I can see some problems in your code. For one, this is a bug:

 array[j]=(char*)malloc(sizeof(p));
 strcpy(array[j],p);

You allocate space for a pointer, not for a copy of the string. Replace these lines with:

 array[j] = strdup(p);

In the for loop, you cannot do this:

 strcat(array[k],"/");
 strcat(array[k],arg[0]);

because array[k] does not have extra room for the strings you tack at the end. You should realloc array[k], which means you need to allocate all arguments so you don't end up leaking memory nor mixing allocated arguments and unallocated ones, albeit it may not matter much since you quickly call exec.

There are probably more problems...