Trying to free memory after calling strcpy causes the program to crash

Question

I understand that a lot of people here complain about strcpy, but I haven't found anything using search that addresses the issue I have.

First off, calling strcpy itself doesn't cause any sort of crash/segmentation fault itself. Secondly, the code is contained within a function, and the first time that I call this function it works perfectly. It only crashes on the second time through.

I'm programming with the LPC1788 microcontroller; memory is pretty limited, so I can see why things like malloc may fail, but not free.

The function trimMessage() contains the code, and the purpose of the function is to remove a portion of a large string array if it becomes too large.

void trimMessage()
{
  int trimIndex;
  // currMessage is a globally declared char array that has already been malloc'd
  // and written to.
  size_t msgSize = strlen(currMessage);

  // Iterate through the array and find the first newline character. Everything
  // from the start of the array to this character represents the oldest 'message'
  // in the array, to be got rid of.
  for(int i=0; i < msgSize; i++)
  {
    if(currMessage[i] == '\n')
    {
      trimIndex = i;
      break;
    }
  }
  // e.g.: "\fProgram started\r\nHow are you?\r".
  char *trimMessage = (char*)malloc((msgSize - trimIndex - 1) * sizeof(char));

  trimMessage[0] = '\f';

  // trimTimes = the number of times this function has been called and fully executed.
  // freeing memory just below is non-sensical, but it works without crashing.
  //if(trimTimes == 1) { printf("This was called!\n"); free(trimMessage); }
  strcpy(&trimMessage[1], &currMessage[trimIndex+1]);

  // The following line will cause the program to crash. 
  if(trimTimes == 1) free(trimMessage);
  printf("trimMessage: >%s<\n", trimMessage);

  // Frees up the memory allocated to currMessage from last iteration
  // before assigning new memory.
  free(currMessage);
  currMessage = malloc((msgSize - trimIndex + 1) * sizeof(char));

  for(int i=0; i < msgSize - trimIndex; i++)
  {
    currMessage[i] = trimMessage[i];
  }

  currMessage[msgSize - trimIndex] = '\0';
  free(trimMessage);
  trimMessage = NULL;

  messageCount--;
  trimTimes++;
}

Thank you to everyone that helped. The function works properly now. To those asking why I was trying to print out an array I just freed, that was just there to show that the problem occurred after strcpy and rule out any other code that came after it.

The final code is here, in case it proves useful to anyone who comes across a similar problem:

void trimMessage()
{
  int trimIndex;
  size_t msgSize = strlen(currMessage);

  char *newline = strchr(currMessage, '\n'); 
  if (!newline) return;
  trimIndex = newline - currMessage;

  // e.g.: "\fProgram started\r\nHow are you?\r".
  char *trimMessage = malloc(msgSize - trimIndex + 1);

  trimMessage[0] = '\f';
  strcpy(&trimMessage[1], &currMessage[trimIndex+1]);

  trimMessage[msgSize - trimIndex] = '\0';

  // Frees up the memory allocated to currMessage from last iteration
  // before assigning new memory.
  free(currMessage);
  currMessage = malloc(msgSize - trimIndex + 1);

  for(int i=0; i < msgSize - trimIndex; i++)
  {
    currMessage[i] = trimMessage[i];
  }

  currMessage[msgSize - trimIndex] = '\0';
  free(trimMessage);

  messageCount--;
}

Answer 1

free can and will crash if the heap is corrupt or if you pass it an invalid pointer.

Looking at that, I think your first malloc is a couple of bytes short. You need to reserve a byte for the null terminator and also you're copying into offset 1, so you need to reserve another byte for that. So what is going to happen is that your copy will overwrite information at the start of the next block of heap (often used for length of next block of heap and an indication as to whether or not it is used, but this depends on your RTL).

When you next do a free, it may well try to coalesce any free blocks. Unfortunately, you've corrupted the next blocks header, at which point it will go a bit insane.

Answer 2

Compare these two lines of your code (I've respaced the second line, of course):

char *trimMessage = (char*)malloc((msgSize - trimIndex - 1) * sizeof(char));
      currMessage =        malloc((msgSize - trimIndex + 1) * sizeof(char));

Quite apart from the unnecessary difference in casting (consistency is important; which of the two styles you use doesn't matter too much, but don't use both in the same code), you have a difference of 2 bytes in the length. The second is more likely to be correct than the first.

You allocated 2 bytes too few in the first case, and the copy overwrote some control information that malloc() et al depend on, so the free() crashed because you had corrupted the memory it manages.

In this case, the problem was not so much strcpy() as miscalculation.

One of the problems with corrupting memory is that the victim code (the code that finds the trouble) is often quite far removed from the code that caused the trouble.

---

This loop:

for(int i=0; i < msgSize; i++)
{
  if(currMessage[i] == '\n')
  {
    trimIndex = i;
    break;
  }
}

could be replaced with:

char *newline = strchr(currMessage, '\n');
if (newline == 0)
    ...deal with no newline in the current messages...
trimIndex = newline - currMessage;

Answer 3

Add this code just before the malloc() call:

// we need the destination buffer to be large enough for the '\f' character, plus
//      the remaining string, plus the null terminator
printf("Allocating: %d  Need: %d\n", (msgSize - trimIndex - 1), 1 + strlen(&currMessage[trimIndex+1]) + 1);

And I think it will show you the problem.

It's been demonstrated time and again that hand calculating buffer sizes can be error prone. Sometimes you have to do it, but other times you can let a function handle those error prone aspects:

// e.g.: "\fProgram started\r\nHow are you?\r".
char *trimMessage = strdup( &currMessage[trimIndex]);

if (trimMessage && (trimMessage[0] == '\n')) {
    trimMessage[0] = '\f';
}

If your runtime doesn't have strdup(), it's easy enough to implement (http://snipplr.com/view/16919/strdup/).

And as a final edit, here's an alternative, simplified trimMessage() that I believe to be equivalent:

void trimMessage()
{
  char *newline = strchr(currMessage, '\n'); 
  if (!newline) return;

  memmove( currMessage, newline, strlen(newline) + 1);

  currMessage[0] = '\f';    // replace '\n'

  messageCount--;
}