Musicode
Musicode

Reputation: 661

C code mysteriously changing the values of a struct (Interesting mysterious code behavior)

I am implementing a history feature for an example UNIX shell in C. I have created an array of structs called History, where each struct element contains an integer (the history number) and the command (an array of strings, each string being an argument to the command). Important parts are denoted ***.

struct HistoryElement
{
  int NumberOfCommandGiven;
  char * command[MAXLINE/2+1];
};  

    int main(void)
    {
    char iBuffer[MAXLINE]; 
    char *args[MAXLINE/2+1];
    int bgrnd;           
    int numberofcommand = 0; //The most recent number given to a command entered (for history).
    int currentposition;
    int historysize = 12;
    struct HistoryElement* History = malloc(historysize*sizeof(struct HistoryElement)); //Create an array of HistoryElements.
    struct HistoryElement blank = { -1, "NothingHere" };
    int j=0;
    //clear out any garbage contents of the history and replace with placeholder element called blank.
    for(j=0;j<historysize;j++) {
      History[j]=blank;
    }

    while (1) {           
      bgrnd = 0;
      printf("IansSh: ");
      fflush(0);
      setup(iBuffer, args, &bgrnd); 

      //Test that the entry was stored correctly -- This outputs "ls" before calling history, but after calling history is outputs "history".  How did History[0] get changed???
      printf("History[0].command equals %s \n",History[0].command[0]);  

      if(strcmp(args[0],"history")==0 || strcmp(args[0],"h")==0) { //***
        //WHY is this section rewriting my array of structs?
        int i;
        for(i=0;i<historysize;i++) {
           if(History[i].NumberOfCommandGiven!=-1)
           {
              if(History[i].NumberOfCommandGiven!=0) {
                  printf("%d : %s \n",History[i].NumberOfCommandGiven, History[i].command[0]);
           }
           else {
               printf("%d : Command was removed due to a more recent command of the same entry. \n", History[i].NumberOfCommandGiven);
           }
        }
       }
      }
    //***
    else {
    printf("Got into final else");
    pid_t errorchecker; 
    errorchecker = fork(); //Create child process
    numberofcommand++;
    //Add the command to history: ***
    struct HistoryElement input;
    input.NumberOfCommandGiven = numberofcommand;
    memcpy(input.command, args, sizeof(args));
    History[numberofcommand-1%historysize] = input;
    //Remove any old entries of that exact command from the history, if they exist:
    int k =0;
    for(k=0;k<historysize;k++)
      {
        if(History[k].command==input.command && History[k].NumberOfCommandGiven!=numberofcommand) 
          {
        History[k].NumberOfCommandGiven=0;
          }   
      }
    if(errorchecker < 0) {
      printf("An error occurred when trying to fork a child process.");
      continue;
    }
    else if(errorchecker==0) {
      //Execute the command, of course:
      execvp(args[0],args);
      // Print the error if an error occurred.
      char* err = strerror(err);
      printf("IansSh error occurred (You most likely entered an invalid command): %s: %s\n", args[0], err);
    }
    if(bgrnd==0) {
      //if specified (ie. an '&' was not included), wait on child process to finish before continuing.
      int child;
      waitpid(errorchecker, &child, 0);
    }
      }
    }
    free(History);
}

In my code, there is a section specifically for calling the command 'history'. This of course is not meant to actually add itself to the history as a command, but instead is meant to display the history line by line. HOWEVER, for some reason, this section of the code is rewriting the values of my array of structs, changing the entry for each command to simply be 'history'. Thus, when I do testing I get something like:

ls -l
(successfully outputs results of ls -l)
ls
(successfully outputs results of ls)
history
1 : history
2 : history

When instead, 1 and 2 should correspond to 'ls -l' and 'ls', respectively, just as they were stored in the struct in main.

Upvotes: 1

Views: 144

Answers (1)

ryanpattison
ryanpattison

Reputation: 6251

Here you are creating a struct where the second field is char *[]

struct HistoryElement input;
input.NumberOfCommandGiven = numberofcommand;
memcpy(input.command, args, sizeof(args));
History[numberofcommand-1%historysize] = input;

EDIT: First off, as @MattMcNabb pointed out, watch your precedence! numberofcommand-1%historysize is evaluated as numberofcommand-(1%historysize) which would have lead to writing off the end of the array (and probably a segfault).

This memcpy copies the pointers that are in args, but does not allocate and copy memory for the actual strings in args. for that you will want to iterate through args (instead of memcpy) and use strdup to create new copies of the strings.

int i;
for (i = 0; args[i] != NULL; ++i) {
    input.command[i] = strdup(args[i]);
    if (input.command[i] == NULL) {
          /* handle allocation error */
    }
}
input.command[i] = NULL;

You will need to free all these strings later too, so you should no longer overwrite a struct without freeing any possible pointers it has allocated.

struct HistoryElement old = History[(numberofcommand-1)%historysize];
int i;
for (i = 0; old.command[i] != NULL; ++i) {
   free(old.command[i]);
   old.command[i] = NULL;
}
History[(numberofcommand - 1) % historysize] = input;

But How does the initial blank fit into this? We can't free "NothingHere" and that initialization was broken anyway:

struct HistoryElement blank = { -1, {NULL} };

Upvotes: 2

Related Questions