40 easy lines, 1 annoying segmentation fault. I really don't know where else to turn now

Question

I apologize if this is a waste of time and/or not what should be on this site, but I'm kind of out of ideas... I'm still a novice at programming, can't get a hold of my teacher for guidance, so... TO THE INTERNET!

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

void months( FILE* monthfp, char** monthGroup );

int main (void){
        FILE *monthfp; /*to be used for reading in months from months.txt*/
        char** monthGroup;
        int i;

        if (( monthfp = fopen ( "months.txt", "r" )) == NULL ){
            printf( "unable to open months.txt. \n" );
            exit ( 1 );
    }

    months( monthfp, monthGroup );

/*test so far*/
    for ( i = 0; i < 12; i++ ){
            printf( "%s", monthGroup[i] );
    }

    fclose( monthfp );


}
void months ( FILE* monthfp, char** monthGroup ){
/*****************************************
    name: months
    input: input file, data array
    returns: No return. Modifies array.
*/
    char buffer[50];
    int count = 0;

    while ( fgets( buffer, sizeof(buffer), monthfp ) != NULL ){
            count++;
            monthGroup =  malloc( count * sizeof ( char* ));
            monthGroup[count] = malloc( sizeof( buffer ) * sizeof( char ));
            strcpy(monthGroup[ count - 1 ], buffer );
    }
}

I'm compiling in C89, everything seems to work, except for a segmentation fault. Any guidance would be very much appreciated.

edit Thanks to everyone who took the time to provide a little bit of insight into something I've been having trouble wrapping my head around. I feel like a little kid in a village of elders in a foreign land. Much appreciation for the courtesy and guidance.

Answer 1

I'm afraid you don't realize how far you are from getting it right. Sit tight, this is going to be long. Welcome to C.

char** monthGroup

All this really means is "a pointer-to-pointer-to-char". However, C has many reasons why you would want to point to something. In your case, the "inner" pointing is so that you can actually point at a sequence of chars in memory (which you colloquially treat as a "string", which C properly does not have), and the "outer" pointing is so that you can point at a sequence of those char*s, and treat that sequence as an "array" (even though it isn't; you're going to dynamically allocate it).

Here's the problem: When you pass in this char** that came from main, it doesn't actually point at anything. "That's fine", you say; "the function is going to make it point at some memory that I'll allocate with malloc()".

Nope.

C passes everything by value. The char** that months receives is a copy of the char** in main's chunk of local variables. You overwrite the pointer (with the result of the malloc call), write some pointers into that pointed-at memory (more malloc results), copy some data into those chunks of pointed-at memory... and then, at the end of the function, the parameter monthGroup (which is a local variable in months) no longer exists, and you've lost all that data, and the variable monthGroup in main is still unchanged at pointing at nothing. When you try to use it as if it points at something, boom you're dead.

So how do we get around this? With another level of pointing, of course, C properly does not have "pass by reference", so we must fake it. We accept a char***, and pass it &monthGroup. This is still a copied value, but it points directly into the local variable storage for that invocation of main (on the stack). That lets us write a value that will be visible in main. We assign the first malloc result to *monthGroup, and write pointers into that storage (*monthGroup[count]), etc.

Except we don't really want to do that, because it's incredibly ugly and confusing and hard to get right. Let's instead do what should be an incredibly obvious thing that you're meant to do and that basic instruction doesn't emphasize nearly enough: use the return value of the function to return the result of the calculation - that's why it's called the return value.

That is, we set up a char** in months (not accepting any kind of parameter for it), return it, and use it to initialize the value in main.

Are we done? No.

You still have some logical errors:

• You re-allocate the "outer" layer within your while-loop. That's clearly not what you want; you're allocating several "strings", but only one "array", so that allocation goes outside the loop. Otherwise, you throw away (without properly deallocating them!) the old arrays each time.

Actually, you do want to do something like this, but only because you don't know in advance how many elements you need. The problem is that the new allocation is just that - a new allocation - not containing the previously-set-up pointers.

Fortunately, C has a solution for this: realloc. This will allocate the new memory, copy the old contents across (the pointers to your allocated "strings"), and deallocate the old chunk. Hooray! Better yet, realloc will behave like malloc if we give it a NULL pointer for the "old memory". That lets us avoid special-casing our loop.

• You're using the value count incorrectly. The first time through the loop, you'll increment count to 1, allocate some space for monthGroup[1] to point at, and then attempt to write into the space pointed at by monthGroup[0], which was never set up. You want to write into the same space for a "string" that you just allocated. (BTW, sizeof(char) is useless: it is always 1. Even if your system uses more than 8 bits to represent a char! The char is the fundamental unit of storage on your system.)

Except not, because there's a simpler way: use strdup to get a pointer to an allocated copy of your buffer.

char** months(FILE* monthfp) {
    char buffer[50];
    int count = 0;
    char** monthGroup = NULL;

    while (fgets(buffer, sizeof(buffer), monthfp) != NULL) {
        // (re-)allocate the storage:
        monthGroup = realloc(monthGroup, count * sizeof(char*));
        // ask for a duplicate of the buffer contents, and put a pointer to the
        // duplicate sequence into the last element of the storage:
        monthGroup[count - 1] = strdup(buffer);
    }

    return monthGroup;
}

Adjusting main to match is left as a (hopefully trivial) exercise. Please also read the documentation for realloc and strdup.

Are we done? No.

You should still be checking for NULL returns from realloc and strdup (since they both attempt to allocate memory, and thus may fail in that way in C), and you still need code to free the allocated memory.

And, as others pointed out, you shouldn't be assuming there will be 12 months. If you could assume that, you wouldn't be dynamically allocating monthGroup in the first place; you'd just use an array. So you need to communicate the size of the result "array" somehow (adding an explicit NULL pointer to the end is one way; another is to do the horribly ugly thing, pass in a char***, and use the return value to count the size).

Answer 2

To me the most obvious of your problems is that you pass char** monthGroup as a parameter by value, then malloc it inside the function months, and afterwards try to use it in the caller function. However, since you passed it by value, you only stored the malloced address in a local copy of monthGroup, which does not change the value of the original variable in main.

As a quick fix, you need to pass a pointer to monthGroup, rather than (a copy of) its current value:

int main (void){
  ...
  char** monthGroup;
  ...
  months( monthfp, &monthGroup );
  ...
}

void months ( FILE* monthfp, char*** monthGroup ){
  ...
  *monthGroup =  malloc( count * sizeof ( char* ));
  ...
}

This is ugly (IMHO there should be no real reason to use char*** in real code) but at least a step in the right direction.

Then, as others rightly mentioned, you should also rethink your approach of reallocating monthGroup in a loop and forgetting about the previous allocations, leaving memory leaks and dangling pointers behind. What happens in the loop in your current code is

// read the first bunch of text from the file
count++;
// count is now 1
monthGroup =  malloc( count * sizeof ( char* ));
// you allocated an array of size 1
monthGroup[count] = malloc( sizeof( buffer ) * sizeof( char ));
// you try to write to the element at index 1 - another segfault!
// should be monthGroup[count - 1] as below
strcpy(monthGroup[ count - 1 ], buffer );

Even with the fix suggested above, after 10 iterations, you are bound to have an array of 10 elements, the first 9 of which are dangling pointers and only the 10th pointing to a valid address.

Answer 3

C has pass-by-value semantics for function calls. This is a fancy way of saying that

int main() {
    int a = 5;
    addOneTo(a);
    printf("%d\n", a);
    return 0;
}

will print 5 no matter what addOneTo() does to its parameter.

In your code, your months() function sets its local variable monthGroup to the value returned by the first malloc(), then throws away that value when it returns.

You have a few choices here on how to fix this problem. You could malloc into monthGroup outside the months() function then pass it in. You could return the monthGroup value. Or you could pass a pointer to monthGroup for pass-by-reference semantics (char***).

In any case, I would encourage you to learn how to use a debugger (e.g. gdb) so you can see why it segfaults next time!

Answer 4

The completed code would be this:

int main (void) 
{
    FILE *monthfp; /*to be used for reading in months from months.txt*/
    char **monthGroup = NULL;
    char **iter;

    if ((monthfp = fopen("c:\\months.txt", "r")) == NULL){
        printf("unable to open months.txt. \n");
        exit(1);
    }

    months(monthfp, &monthGroup);

    iter = monthGroup;

    /* We know that the last element is NULL, and that element will stop the while */
    while (*iter) {
        printf("%s", *iter);
        free(*iter);
        iter++;     
    }

    /* Remember that you were modifying iter, so you have to discard it */
    free(monthGroup);

    fclose(monthfp);
}

void months(FILE *monthfp, char ***monthGroup)
{
/*****************************************
    name: months
    input: input file, data array
    returns: No return. Modifies array.
*/
    char buffer[50];
    int count = 0;

    while (fgets(buffer, sizeof(buffer), monthfp) != NULL){
        count++;

        /* We realloc the buffer */
        *monthGroup = (char**)realloc(*monthGroup, count * sizeof(char**));

        /* Here I'm allocating an exact buffer by counting the length of the line using strlen */
        (*monthGroup)[count - 1] = (char*)malloc((strlen(buffer) + 1) * sizeof( char ));
        strcpy((*monthGroup)[count - 1], buffer);
    }

    /* We add a terminating NULL element here. Other possibility would be returning count. */
    count++;
    *monthGroup = (char**)realloc(*monthGroup, count * sizeof(char**));
    (*monthGroup)[count - 1] = NULL;
}

As said by others a char*** is ugly.

Answer 5

Your problem lies in the months function, specifically your understanding of how memory works.

Looking at your code:

monthGroup =  malloc( count * sizeof ( char* ));

This line allocates a chunk of memory which is equivalent to an array of char * of size count.

monthGroup[count] = malloc( sizeof( buffer ) * sizeof( char ));

Here, a buffer is allocated of size sizeof (buffer) (the sizeof (char) is unneccesary). This is one problem here: you are assigning it to monthGroup[count]. Arrays in C are zero-base, which means that the array:

int array [3];

has elements:

 array [0], array [1] and array [2]

array [3] is outside the memory of the array. So monthGroup[count] is also outside the memory of the array. You want monthGroup[count-1] instead. This will write to the last element in the array.

The second problem is that every time you do the first allocation, you lose the previously allocated data (this is know as a memory leak) and the data it contained.

To fix this, there are two approaches.

1. When allocating the array, copy the contents of the old array to the new array:

   oldarray = monthGroup; monthGroup = malloc (count * sizeof (char *)) memcpy (monthGroup, oldarray, count-1 * sizeof (char *)); free (oldarray); monthGroup [count-1] = ....

   or use realloc.

2. Use a linked list. A lot more complex this one but has the advantage of not requiring the arrays to be copied every time a new item is read.

Also, the monthGroup parameter doesn't get passed back to the caller. Either change the function to:

char **months (FILE *fp)

or:

void months (FILE *fp, char ***ugly_pointer)

Finally, the caller currently assumes that there are 12 entries and attempts to print each one out. What happens if there are fewer than 12, or more than 12? One way to cope is to use a special pointer to terminate the monthsGroup array, a NULL would do nicely. Just allocate one extra element to the array and set the last one to NULL.

Answer 6

The principal error that I see immediately, is that your allocation for monthGroup will never make it back into your main.