free() caused segmentation fault

Question

In this program, there is a segmentation fault. The program can print out "loop end" successfully and segmentation fault appeared after "loop end", which means there is not error in read_name function. But I could not figure out any error in my free_memory function. Could anyone help me figure out? Thank you.

input file:

9
Clinton, Hillary R.
Bonds, Bobby S.
Bonds, Barry L.
Clinton, William I.
Clinton, Chelsea T.
Bush, Laura M.
Bush, George W.
Bush, Jenna F.
Bush, Barbara G.

program:

#include <stdio.h> 
#include <malloc.h>
#include<string.h>

void alloc(char ***surname, char ***first, char **mid_init, int num);
void read_names(FILE *inp, char **surname, char **first, char *mid_init, int num );
void free_memory(char **surname, char **first, char *mid_init, int num);

int main(int argc, char *argv[])
{
  int num = 0;
  char **surname, **first, *mid_init;
  FILE *inp = fopen(argv[1], "r");  
  FILE *outp = fopen(argv[2], "w");
  char array[79];

  fscanf(inp, "%d", &num);
  printf("%d\n", num);

  fgets(array, 79, inp);

  alloc(&surname, &first, &mid_init, num);
  read_names(inp, surname, first, mid_init, num);
  free_memory(surname, first, mid_init, num);

  fclose(inp);
  fclose(outp);

  return 0;
}

void alloc(char ***surname, char ***first, char **mid_init, int num)
{
  int i;

  *surname = (char**)malloc(num * sizeof(char*));
  *first = (char**)malloc(num * sizeof(char*));
  *mid_init = (char*)malloc(num * sizeof(char));

  for(i=0; i<num; i++)
  {
    (*surname)[i] = (char*)malloc(15*sizeof(char));
    (*first)[i] = (char*)malloc(10*sizeof(char));
  }
}

void read_names(FILE *inp, char **surname, char **first, char *mid_init, int num )
{
  char *token, array[79];
  char delim[6] = ", .\n";
  int i=0;

  fgets(array, 79, inp);
  printf("loop begins\n");

  for(i=0; i<num; i++)
  {
      fgets(array, 79, inp);
      printf("%s", array);

       token = strtok(array, delim);
    strcpy( (surname[i]), token);
    printf("%s   ", (surname[i]));

    token = strtok(NULL, delim);    
    strcpy( (first[i]), token);
    printf("%s  ", (first[i]));

    token = strtok(NULL, delim);
    *mid_init = token[0];
    printf("%s\n", mid_init);

    printf("\n\n");

  }
     printf("\nloop ends\n");
}

void free_memory(char **surname, char **first, char *mid_init, int num)
{
  int i;

  for(i=0;i<num;i++)
  {
    free((surname)[i]);
    free((first)[i]);
  }

  free(surname);
  free(first);
  free((mid_init));
}

Answer 1

First off, you're limiting yourself to 14-character first names and 9-character last names, so that would be the first thing I'd check, that your names aren't longer than this.

If they are, you'll probably corrupt the memory arena when copying them.

One way to check this is to simply print the length of token every time you set it, such as:

token = strtok(array, delim);
printf ("DEBUG: token length is %d\n", strlen (token));

Keep in mind that corruption does not necessarily have a visible immediately or even ever. In this case, what's most likely happened is that you've overwritten a vital piece of inline control information in the memory arena, such as a memory block size or a pointer to another memory block.

However, there's no code actively checking for that when you write to memory so it's probably only found when you next try to do a memory allocation or de-allocation call.

Your next call like this after the corruption is your free calls and that's almost certainly where it's being found, because the arena is corrupt.

Bottom line, writing beyond the end of allocated memory is undefined behaviour. That means you shouldn't do it.

---

If it turns out your names aren't too long (as you state in a comment), you need to then ask yourself why you have a superfluous fgets(array, 79, inp); in your code. I understand why it's needed in main so as to move to the next line after inputting the line count with a call to fscanf. And that one does its job well.

However, you have another one at the start of read_names which effectively throws away the first name in your list. That's going to cause problems because, while your code thinks there are X names in the file, you've thrown away the first one meaning that there are only X - 1 remaining. You can tell this because, when you begin to print out the names, the first one from the file appears to be missing.

If you remove the fgets at the start of read_names, you should find it's okay.

As an aside, there's a couple of other changes I'd make to the code. First you really should check all those malloc calls in case one of them fails. That's the general rule for all functions that can fail when you rely later on them not having failed.

Second, I'm not really a big fan of ever multiplying by sizeof(char) - this is guaranteed by the standard to always be 1, so multiplying by it clogs up the code and makes it less readable.

Answer 2

Try replacing

token = strtok(NULL, delim);
*mid_init = token[0];
printf("%s\n", mid_init);

with

token = strtok(NULL, delim);
mid_init[i] = token[0];
printf("%c\n", mid_init[i]);

When mid_init memory chunk is filled with garbages without any null in it, 'printf("%s\n", mid_init);' might read beyond the data segment causing segmentation fault. But @paxdiablo's answer has a much better chance to be the case here.

@Bruce, segmentation fault doesn't always appear at the exact spot it happened.

Answer 3

I don't know why you are getting a segmentation fault, but if I was writing this, I would try and make it a bit simpler (I don't think you are doing yourself any favors) - it makes no sense to pass around something like char ***surname.

This is only my personal opinion, but I would do something like this:

#include <stdio.h>
#include <malloc.h>

typedef struct {
    char **data;
    unsigned int count;
    unsigned int actualSize;
} StringArray;

void StringArray_init(StringArray *array)
{
    array->count = 0;
    array->actualSize = 0;
}

void StringArray_add(StringArray *array, char* value)
{
    if (array->actualSize == 0)
    {
        array->data = (char**)malloc(sizeof(char*)* 4);
        array->actualSize = 4;
    }
    else 
    {
        if (array->count >= array->actualSize)
        {
            array->actualSize *= 2;

            array->data = (char**)realloc(array->data,sizeof(char*) * array->actualSize);
        }
    }
    array->data[array->count] = value;
    array->count++;
}

char* StringArray_get(StringArray *array, unsigned int position)
{
    if (position < array->count)
        return array->data[position];
    else
        return 0;
}

void StringArray_clear(StringArray *array)
{
    if (array->count >0) free(array->data);
    array->count = 0;
    array->actualSize = 0;
}

int main ()
{
    StringArray surname;
    StringArray_init(&surname);
    StringArray_add(&surname, "Smith");
    StringArray_add(&surname, "Jones");

    for(int i=0;i<surname.count;i++)
    {
        printf("%s\n", StringArray_get(&surname,i));
    }

    StringArray_clear(&surname);

}

What the above code is doing is allocating memory when you add a value, but instead of allocating space for one item, it adds enough for four. If you get to the point where you add a fifth, it will then double the space to 8 items. This method should help with memory fragmentation. I'm also using a structure, which just makes it a bit easier to pass this array around.

I could also do something like this if I wanted to allocate memory for the strings (just include the string.h header):

int main ()
{
    StringArray surname;
    StringArray_init(&surname);

    char *name = (char*)malloc(sizeof(char) * 6);
    strcpy(name,"Smith");       
    StringArray_add(&surname, name);

    name = (char*)malloc(sizeof(char) * 6);
    strcpy(name,"Jones");       
    StringArray_add(&surname, name);

    for(int i=0;i<surname.count;i++)
    {
        printf("%s\n", StringArray_get(&surname,i));
    }

    // Free memory
    for(int i=0;i<surname.count;i++)
    {
        char *name = StringArray_get(&surname,i);
        if (name != NULL) free(name);
    }
    StringArray_clear(&surname);
}

The size of each name is 6 because there are 5 characters and an extra one for 0, which is the string terminator.

Sorry if this doesn't directly answer your question, but hope it helps.

Answer 4

Bruce,

In your data file, you need a blank line between the number and list of names because of the first fgets() at the beginning of read_names() consumes the second line. Because the program skipped the second line, it could read only 8 names and the last line read was a blank line, which caused strtok to return a null and the next strcpy tried to read from address 0, which is, of course, a segmentation fault. And in my machine, the fault happened before printing "loop ends".

You need to check the return values of function calls (strtok in this case) for possible errors. Otherwise you will be wasting a lot of time debugging like this.