Writing my own shell in c - free() causing problems

Question

I'm trying to write my own shell, but something is not working with the allocations and free. I reviewed my code over and over, and I cannot understand why my free function is causing me problems... When I don't use it, everything works fine, but when I use it, the code stops working after the second iteration... I would really appreciate your help...

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <limits.h> //for PATH_MAX - longest path name permited in linux
#include <sys/wait.h>
#include <fcntl.h>

typedef struct{
char **parametersArray; //this array contains the command and the parameters
int size_of_array; //the number of strings in the array
int toFileFlag; //if we wnat to write to file
char *toFile; //name of file to write to
int fromFileFlag;//if we wnat to read from file
char *fromFile; //name of file to read to
}UserInput;

int runInBackground = 0; //is command running in background? if so, runInBackground=1;

//********************************************************************************************************************
//functions list:

UserInput* inputTokenization(char *a); //recieve string of the user input, and returns pointer to struct of UserInput.   
void execCommand(UserInput *user_input); //exec the requestd command with the parameters given 
void free_All_Allocations(UserInput *userinput); 

 //*******************************************************************************************************************
int main(int argc, char *argv[])
{
    char userInputTxt[LINE_MAX]; //the line the user enters (hopefully command+parameters)   
    UserInput *u_i;   
    int i = 0;

    while(1)
    {
        i = 0;
        printf("\033[1;35m"); //print in with color (purple)
        printf("### "); //### is the prompt I chose    
        fflush(stdout);
        memset(userInputTxt, 0, LINE_MAX); //cleaning array from previous iteration 
        read(0, userInputTxt, LINE_MAX); 

        if(strcmp(userInputTxt, "exit\n") == 0) //exit the program if the user enters "exit"       
            exit(EXIT_SUCCESS);

        u_i = inputTokenization(userInputTxt); //parsing the char array userInputTxt
        execCommand(u_i); 
        free_All_Allocations(u_i);
    }

}

UserInput* inputTokenization(char *a)
{   
    int i=0, size;    
    size = strlen(a);
    UserInput *user_input = (UserInput*)malloc(sizeof(UserInput)*1); 
    if(user_input == NULL)
     {
        perror("failed to allocate memory");
        exit(EXIT_FAILURE); 
    }


    user_input->fromFileFlag = 0;
    user_input->toFileFlag = 0;
    user_input->size_of_array = 2;

    //counting how many token we have
    while(i<size)
    {
        if(a[i] == ' ')
            (user_input->size_of_array)++;
        if (a[i] != '<' || a[i] != '>' )
            break;
        i++;
    }
    printf("%d\n", user_input->size_of_array);

    //we don't want to change original array(a), so we'll copy a to tmp and use tmp
    char *tmp = (char*)malloc(size+1);
    if(tmp == NULL)
     {
        perror("failed to allocate memory");
        exit(EXIT_FAILURE); 
    }
    strncpy(tmp, a, size-1);

    //we'll allocate array of arrays. It's size: number of tokens in the original array, even though we might not use all of it-
    //some tokens might be name of file to read or write to
    user_input->parametersArray = (char**)malloc(user_input->size_of_array);
    if(user_input->parametersArray == NULL)
     {
        perror("failed to allocate memory");
        exit(EXIT_FAILURE); 
    }

    i=0;    
    char* token = strtok(tmp, " ");   
    user_input->parametersArray[i] = (char*)malloc(strlen(token)+1);
    if(user_input->parametersArray[i] == NULL)
     {
        perror("failed to allocate memory");
        exit(EXIT_FAILURE); 
    }
    strcpy(user_input->parametersArray[i], token);
    i++; 

    while(token != NULL)
    {
        token = strtok(NULL, " ");
        if(token !=NULL)
        {
            if(strcmp(token, "<") != 0 && strcmp(token, ">") !=0 && strcmp(token, "&") != 0)
            {
                user_input->parametersArray[i] = (char*)malloc(strlen(token)+1);
                if(user_input->parametersArray[i] == NULL)
                {
                    perror("failed to allocate memory");
                    exit(EXIT_FAILURE); 
                }
                strcpy(user_input->parametersArray[i], token);                
                i++;
                continue;
            }

            if(strcmp(token, "<") == 0)
            {
               user_input->fromFileFlag = 1;
               token = strtok(NULL, " ");
               if(token !=NULL)
               {
                   user_input->fromFile = (char*)malloc(strlen(token)+1);
                   if(user_input->fromFile == NULL)
                    {
                        perror("failed to allocate memory");
                        exit(EXIT_FAILURE); 
                    }
                   strcpy(user_input->fromFile, token);
               }               
            }

            if(strcmp(token, ">") == 0)
            {
               user_input->toFileFlag = 1;
               token = strtok(NULL, " ");
               if(token != NULL)
               {
                   user_input->toFile = (char*)malloc(strlen(token)+1);
                   if(user_input->toFile == NULL)
                    {
                        perror("failed to allocate memory");
                        exit(EXIT_FAILURE); 
                    }
                   strcpy(user_input->toFile, token);
               }               
            }
            if(strcmp(token, "&") == 0)
            {
               runInBackground = 1;
               break;                            
            }

        }
    }        

    user_input->parametersArray[i] = NULL;

    free(tmp);
    return user_input;       
}

void execCommand(UserInput *user_input)
{
   pid_t pid;
   int status;   

    pid = fork();

    if(pid == -1) //fork failed
    {
        perror("fork() failed");
        exit(EXIT_FAILURE);
    }
    if(pid == 0) //child process
    {        
        if(user_input->fromFileFlag == 1) //if we have file to read from
        {
            close(0);
            if(open(user_input->fromFile, O_RDONLY) == -1) 
            {                
                perror("open file to read failed");
                exit(EXIT_FAILURE);
            }
        }
        if(user_input->toFileFlag == 1) //if we have file to write to
        {
            close(1);
            if(open(user_input->toFile, O_WRONLY | O_CREAT, 0766) == -1)
            {
                perror("open file to write failed");
                exit(EXIT_FAILURE);
            }
        }

        if(execvp(user_input->parametersArray[0], user_input->parametersArray) == -1)
        {
            perror("execvp() failed");
            exit(EXIT_FAILURE);
        }
    }

    if(runInBackground == 0) //as long as this is the only command to execute, 
        waitpid(pid, &status, 0); //wait until chile process (execvp) finish. Otherwise, father process go again, and chile process run in background   
}

void free_All_Allocations(UserInput *userinput)
{
    int i=0;

    while(userinput->parametersArray[i] != NULL)
     {                
         free(userinput->parametersArray[i]);
         i++;        
     }    

     free(userinput->parametersArray);   
     if(userinput->fromFileFlag == 1)
         free(userinput->fromFile);
     if(userinput->toFileFlag == 1)
        free(userinput->toFile);       
     free(userinput);    
}

Answer 1

I recommend the use of valgrind.

Compile with your code with the flag -ggdb3 and then execute valgrind with your program. It'll show you all the invalid reads and writes during the execution of the program. Not only that, it'll tell you exactly in what line they happen and the corresponding function call trace.

This question is a great starting point if you're a beginner to valgrind.

Answer 2

One problem is that your "counting how many token we have" is wrong. It will exit on the first iteration, because the condition a[i] != '<' || a[i] != '>' will always be true. I think the comparison you want is a[i] == '<' || a[i] == '>', which will exit the loop if either of those characters is found. This will result in user_input->size_of_array being 2 (or 3, if the first character in a is a space). Later, when you actually pull tokens from the string, you write past allocated memory if there are more than two (or possibly three) tokens.

This counting loop itself is flawed, because it counts differently than the loop that actually extracts the tokens. (For example, if a token is "a>b", your counter would stop but the tokenizer loop would treat that as a token and keep going.) It would be better to use the same sort of loop to count the token, using strtok, or better yet use a way to dynamically resize your parameters array so you only need to make one pass and don't need to count. With either loop, multiple adjacent spaces result in an empty token.

But that isn't all. The count being wrong isn't currently an issue because of the next problem: the allocation for user_input->parametersArray uses the wrong size. Since you want user_input->size_of_array elements, you should use

user_input->parametersArray = malloc(user_input->size_of_array * sizeof(char *));

or, to avoid problems with the proper type, you can go with

user_input->parametersArray = malloc(user_input->size_of_array * sizeof(*user_input->parametersArray));

Note that I have removed the cast of the return value from malloc. It is not necessary in C, and can lead to subtle problems if the type used in the cast is not the correct one.

Another problem is the call to strncpy. Since the length of the string is size, the terminating nul character will not be copied. Since you now the buffer is big enough, you should just use strcpy (or strcpy_s if your compiler is new enough to support it).