First time using Malloc, and program crashes

Question

the program is intended to recieve 'n' number of students and their names and sort them (havent got to the sorting part). I cant figure out why my program crashes when i test it. this is the code:

#include<stdio.h>
#include <stdlib.h>
#define MaxNameLen 100
int main() {

    int n;
    scanf("%d", &n);
    char *names;
    char **pointerToNames = (char **) malloc(n * sizeof(char));
    if (pointerToNames == NULL)
        return 0;

    int i;

    for (i = 0; i <= n; i++) {
        names = (char *) malloc(MaxNameLen);
        gets(names);
        pointerToNames[i] = names;
    }

    for (i = 0; i < n; i++) {
        free(pointerToNames[i]);
        free(names);
    }
}

@haccks http://www.cplusplus.com/reference/cstdio/gets/ – Inisheer Jan 11 '14 at 11:54 — Inisheer, Jan 11 '14 at 11:54
@Inisheer; Sorry! `gets` become history now in C. – haccks Jan 11 '14 at 11:57 — haccks, Jan 11 '14 at 11:57

score 5 · Answer 1 · edited May 23 '17 at 12:03

You have three problems. The first is that you don't allocate enough entries for the "array":

malloc(n * sizeof(char))

Should be

malloc(n * sizeof(char*))

The second problem is the reading loop:

for (i = 0; i <= n; i++) {

Here the loop condition will cause you to loop one time to many, causing you to write beyond what you allocate (if you fix the first problem). The condition in the loop should be i < n like in the next loop.

The third problem is that you free the last string repeatedly in the loop

for (i = 0; i < n; i++) {
    free(pointerToNames[i]);
    free(names);
}

As you assign names to pointerToNames[i] in the loop above, when that loop is done names will point to the last string you read, so names and pointerToNames[n - 1] will point to the same string.

Two other problems include that you are not freeing the actual pointerToNames memory you first allocate. And that you should not use gets (it has long been deprecated, and even removed in the latest standard). Use fgets (or gets_s) instead.

Also, don't cast the return of malloc.

mcleod_ideafix · Answer 2 · 2014-01-11T12:00:46.073

Your first malloc is meant to allocate memory for N pointers to chars, but you are allocating N chars, which is probably much less memory.

Change this line:

char **pointerToNames = (char **) malloc(n * sizeof(char));

To this:

char **pointerToNames = malloc(n * sizeof(*pointerToNames));

Besides:

for (i = 0; i < n; i++) {
        free(pointerToNames[i]);
        free(names);

This frees memory for the indicated index, but you also free names, which happened to be the temporary pointer you used to allocate memory, and then you assigned the current value of name to each eement. So, there is no need to free names.

Maybe you find simpler to change your second part (the code that allocates memory for each element of your array) to this:

for (i = 0; i < n; i++) {
        pointerToNames[i] = malloc(MaxNameLen);
        if (pointerToNames[i])
          fgets(pointerToNames[i], MaxNameLen-1, stdin);
    }

So names is no longer used and instead of gets(), use fgets(). Note that the for loop goes from 0 to N-1, not to N. pointerToNames[N] is not a valid array element.

First time using Malloc, and program crashes

2 Answers2