C Strings storing values into array without over writting

Question

I am trying to get the last 10 commands entered in my program. The issue I am having is that I take arguments from the command line and store that into my array but I keep over writing the values in my array because they are storing a pointer not an actual character array value. I want to know how to take the current character array (going to be changing with each command entered) and store that value into an array. I know its a pointer issue but I'm unsure of how to fix this

My code is below. my char array history[] goes as follows with the following input:

input a history[0] = a input b history[0] = b history[1] = b input c history[0] = c history[1] = c history[2] = c

#define MAX 256
#define CMD_MAX 10
#define HISTORY_MAX 10

int make_history(char *history[], int history_counter, char *input_line)
{
  if(history_counter < HISTORY_MAX)
  {
    history[history_counter] = input_line;
  }
  return 1;
}



int make_tokenlist(char *buf, char *tokens[])
{
   char input_line[MAX];
   char *line;
   int i,n;

   i = 0;
   line = buf;
   tokens[i] = strtok(line, " ");

   do
   {
    i++;
    line = NULL;
    tokens[i] = strtok(line, " ");
   }

   while(tokens[i] != NULL);

   return i;
 }

int main()
{
  char input_line[MAX], *tokens[CMD_MAX], *history[HISTORY_MAX];
  int i, n, history_counter;

  while(1)
  {
    printf("abc> ");

    if (gets(input_line) != NULL)
    {
     n= make_tokenlist(input_line, tokens);
    }
    else
    {
     printf("error?");
     return 1;
    }

    make_history(history,history_counter,input_line);

    if(strcmp(tokens[0],"history")==0)
    {
     for (i = 0; i < HISTORY_MAX; i++)
     {
       if(history[i]!=NULL)
       {
         printf("%d. %s \n", i, history[i]);
       }
     }
    } 

    for (i = 0; i < n; i++)
    {
     printf("extracted token is %s\n", tokens[i]);
    }

    history_counter++; //increment history counter
  }
}

have you looked into using a library for this? something like the readline library? web.mit.edu/gnu/doc/html/rlman_2.html — lbrendanl
– lbrendanl, Commented Jan 26, 2013 at 2:02
you don't allocate memory for 10*256 char at all, use 2d array or malloc and free — 0x90
– 0x90, Commented Jan 26, 2013 at 2:35

Laogeodritt · Accepted Answer · 2014-12-01 07:51:07Z

The problem (which I think you understand) is that you're storing a pointer to the same character array as input_line. In other words, the string inside history and input_line are the same data.

The solution is to make a copy of the input_line and storing that into the history. A naïve implementation (which will not work) is as follows:

int make_history(char *history[], int history_counter, char *input_line)
{
    if(history_counter < HISTORY_MAX)
    {
        char input_copy[MAX];
        strcpy(input_copy, input_line);
        history[history_counter] = input_copy;
    }
    return 1;
}

Now, you're making a copy of the line into memory that isn't used anywhere else (input_copy). That should work, right? The problem is that declaring a variable like that (called static allocation) stores the variable's data on the stack. When a function returns, all the data that was put in the stack by that function is destroyed. When make_history() returns, input_copy is destroyed, and since history[history_counter] was pointing to the same array as input_copy, it's now pointing to a bit of memory that's probably going to be used for other purposes by the program and won't contain your copy of the input line any more.

You can use dynamic allocation to get around the issue of statically allocated memory existing only in the scope it is declared in. malloc() allocates memory on what's called the heap (in C; in C++ you'd use new). Data in the heap doesn't get destroyed while the program is running unless you tell it to free the memory yourself. Example:

int make_history(char *history[], int history_counter, char *input_line)
{
    if(history_counter < HISTORY_MAX)
    {
        char *input_copy = (char *)malloc(MAX * sizeof(char));
        strcpy(input_copy, input_line);
        history[history_counter] = input_copy;
    }
    return 1;
}

This will work, since data in the heap (allocated by malloc()) will survive after make_history() returns and until either you free that memory yourself, or the program ends. Note how I use sizeof(char)—this is because primitive types (e.g. int, char) can differ in size according to the kind of processor you're compiling for, the compiler you're using, etc. (EDIT: Just a note, sizeof(char) == 1 is defined by the C specification (I think all revisions), and all other data type sizes are given in multiples of char.)

But what if:

// history_counter == 0
make_history(history,history_counter,input_line);
// ...
// later, when history_counter == 0 again somehow
make_history(history, history_counter, another_input_line);

All the second make_history() did was change a pointer from the copy of input_line to the copy of another_input_line! It never frees the memory of the first input_line, and now all the pointers to it have disappeared. That's called a memory leak (and it would be wonderful if they were all that simple!). If this were possible in your program (say, you made history_counter loop back to 0), you would want to fix this. After you allocated that memory the first time, you can just reuse it, right?

int make_history(char *history[], int history_counter, char *input_line)
{
    if(history_counter < HISTORY_MAX)
    {
        if(history[history_counter] == NULL)
        {
            history[history_counter] = (char *)malloc(MAX * sizeof(char));
        }
        strcpy(history[history_counter], input_line);
        history[history_counter] = input_copy;
    }
    return 1;
}

Now you're checking if you had already stored a copy of a line in history[history_counter] before, and if not you malloc() the memory first. Either way, after that you copy input_line into the history array. (There's another problem with your code that will break this; see below.)

Another thing you should know about dynamic allocation, since that seems to be the subject we're on, is that to free memory that you allocated using malloc(), use free(). For C++, to free memory allocated using new, use delete.

Another possible solution, as mentioned by a comment to your question: you could statically allocate 10*256*sizeof(char) bytes in main() right off the bat. It's a simpler solution, but uses up that memory all the time instead of just when needed. Your example is simple and doesn't require that much memory, but in bigger/more complicated programs, that difference can matter.

By the way, a few issues in your code other than the question:

You never initialize history_counter in main(), so its initial value is undefined when you call make_history(). You should treat an uninitialized variable (declared, but no value assigned) as random—in reality, the value depends on processor/system, compiler, OS, and is nothing you should rely on. Therefore, the second line of main() should be:

int i, n, history_counter = 0;

Similarly, you declared history as:

char *history[HISTORY_MAX];

and you don't initialize that array of pointers to NULL values. Later on you check:

if(history[i] != NULL)

but you have no guarantee that the default value of history[i] is NULL for all i. You should initialize the history array to all NULL values first. My last example above for my actual answer also relies on this initialization being done, and bad things could happen if you try to free() an unallocated address.

Collectives™ on Stack Overflow

C Strings storing values into array without over writting

1 Answer 1

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

Comments

Your Answer

Sign up or log in

Post as a guest

Related