History feature using Linked List giving Segmentation Fault

Question

#include<stdio.h>
#include<unistd.h>
#include<sys/types.h>
#include<stdlib.h>
#include<string.h>
#define MAX_LINE 80 /* Maximum  length of command*/



struct List
{
char *commandName;
struct List *next;
struct List *prev;
};

struct List *head=NULL;
struct List *curr=NULL;

void accessList(int);
void addToList(char *);
struct List *createNode(char *);

I want to add history feature to my shell. For that I am creating a linked list using structs. The command which is entered on my shell gets divided into tokens and the first token should get stored in linked list (doubly).Below is the main() function.

int main(int argc, char *argv[])
{
 char *args[MAX_LINE/2+1]; /* Command Line Argument*/
 int should_run=1, status, i, num;
 pid_t pid;
 char str[41], teststr[41];
 const char delimiter[2]=" ";
 char *token;
 while(should_run)
 {
i=0;
printf("osh>");
fflush(stdout);

fgets(str, sizeof str, stdin);     //reads user input
str[strlen(str)-1]='\0';
token=strtok(str, " ");    //breaks them into tokens using the following while loop

    while(token)
    {
       args[i]=strdup(token);
       printf("args[%d]=%s\n", i, args[i]);
       i++;
       token=strtok(NULL, " ");
    }
    if(pid<0)          // error in creating child process
    {
       printf("\tError in creating child process\n");
    }       
    else if(pid==0)        //child process will execute this block
    {
       printf("\tChild running it's block\n");
       addToList(args[0]);     //store args[0] in Linked List
       execvp(args[0], args);   //args[0] contains name of command
                 exit(1);
    }
    else               //parent process will execute this block
    {
       pid=wait(&status);
       printf("\tNow Parent resumes\n");

    if(!strcmp(args[0], "exit"))
    {
            should_run=0;
    }
}
 }
 printf("\tNo. of latest commands you want to view from history=\n");
       scanf("%d", &num);
       accessList(num);            //function to traverse list
 return 0;
} 
void accessList(int number)
{
 int i;
 struct List *temp=(struct List*)malloc(sizeof(struct List));
 temp=NULL;
 temp=curr;
 for(i=number;i>0;i--)
 {
  printf("%d %s", &i, temp->commandName);
  temp=temp->prev;
 }
}
void addToList(char *cmd)
{
 struct List *ptrNode=(struct List*)malloc(sizeof(struct List));
 ptrNode=NULL;
 ptrNode=createNode(cmd);
 if(head->next==NULL)
 {
  head->next=ptrNode;
  ptrNode->prev=head;
  curr=ptrNode;
 }
 else
 {
  curr->next=ptrNode;
  ptrNode->prev=curr;
  curr=curr->next;
 }
}
struct List *createNode(char *cmd)
{
 struct List *node=(struct List*)malloc(sizeof(struct List));
 node->commandName=cmd;
 node->next=NULL;
 node->prev=NULL;
 return node;
}

When I run this program, my shell works fine but when ask user to input number of latest commands they want to view from history, it gives me segmentation fault.

Two things: First [don't cast the return value of `malloc` in C](http://stackoverflow.com/a/605858/440558). Secondly, you have a memory leak in `addToList`, you first call `malloc` then overwrite that pointer *twice*. — Some programmer dude, Feb 23 '14 at 19:44
As for the list access, the first time you add a node to the list `head` will be `NULL`, so you have a `NULL` pointer access error there. — Some programmer dude, Feb 23 '14 at 19:45
As for your problem, build a debug version and run in a debugger. — Some programmer dude, Feb 23 '14 at 19:48
You have another memory leak in accessList, temp is allocated on heap and after you set it to null. You should have `struct List *temp=curr;' — joy, Feb 23 '14 at 19:52

Dmitri · Answer 1 · 2014-02-23T20:03:37.950

You have an error in one of your printf()s:

printf("%d %s", &i, temp->commandName);

should be:

printf("%d %s", i, temp->commandName);

...since you need the value of i, not it's address.

Also, you're calling addToList() after the fork(), in the child process -- which won't affect the list in the parent.

You also don't handle the case where head is NULL (as when adding the first item) in addToList(), so when you do call it you'll probably get a segfault.

Also, in the line where you declare temp in accessList():

struct List *temp=(struct List*)malloc(sizeof(struct List));

...you allocate a struct which you then discard (memory leak) right afterward. You don't need the malloc() if you're pointing to nodes that already exist, and you don't need to initialize the value here if you're assigning it one right after -- though if you do, you should just initialize it to NULL.

I used what all was hinted above and tried to remove as many memory leaks as I can but now I am getting error: dereferencing pointer to incomplete type. — Malhar, Feb 23 '14 at 22:14

History feature using Linked List giving Segmentation Fault

1 Answers1