Deallocation of memory in arrays

Question

I've done a program that opens the file (read binary), and saves all the words(in the file) in an array of char (allocated dynamically in base of the length of the word).
This is the code:

char **leggi_stringhe(const char *filename, size_t *size) {
    FILE *f = fopen(filename, "rb");
    if (f == NULL) {
        *size = 0;
        return NULL;
    }

    int x;

    if (fread(&x, 1, 4, f) != 4) {
        *size = 0;
        return NULL;
    }

    char **stringhe = malloc((x) * sizeof(char));

    for (int i = 0; i < x; i++) {
        int z = 0;
        if (fread(&z, 1, 4, f) != 4) {
            *size = 0;
            return NULL;
        }

        stringhe[i] = malloc((z)* sizeof(char));
        if (fread(stringhe[i], 1, z, f) != z) {
            *size = 0;
            return NULL;
        }
        stringhe[i][z] = 0;
    }
    *size = x;
    fclose(f);
    return stringhe;
}

int main(void) {
    size_t t;
    char **a = leggi_stringhe("file1.bin", &t);

    for (int i = 0; i < t; i++)
        free(a[i]);
    free(a);;
}

The program works, but i have problems with memory deallocation. After calling of leggi_stringhe function, the variable a contains:

a[0] = "first"
a[1] = "second"
a[2] = "third"

but when i'm trying to deallocate the whole a variable as i wrote, the debugger stops with a warning.
I was inspired by this question for writing my code Using Dynamic Memory allocation for arrays, but do not understand why I get this error when i try to deallocate.

`fread(&z, 1, 4, f)`? Why do you assume that `z` is four bytes? You also implicitly assume that your data file comes from a machine with the same endianness as the one you're processing on. — Andrew Henle, Feb 15 '17 at 14:07
This code has other fundamental problems apart from the mentioned bugs. See [Correctly allocating multi-dimensional arrays](http://stackoverflow.com/questions/42094465/correctly-allocating-multi-dimensional-arrays). — Lundin, Feb 15 '17 at 14:14

score 4 · Accepted Answer · answered Feb 15 '17 at 14:06

4

Your initial call to malloc is wrong. You allocate space for x characters, not for pointers to char.

Your second call inside the loop is wrong to, as you don't allocate space for the terminator.

Lastly, and unrelated to the problems you ask about, but if the fread calls inside the loop fails, you will have memory leaks.

answered Feb 15 '17 at 14:06

Some programmer dude

400,186
35
402
621

3

To the OP: @M41Npain and please check that `malloc()` did not return `NULL`. It might be too much for this code, but it's a habit that you MUST develop – Iharob Al Asimi Feb 15 '17 at 14:08
@IharobAlAsimi cplusplus.com say *If the function failed to allocate the requested block of memory, a null pointer is returned.* what it is referring? – Amarildo Feb 15 '17 at 14:14
1

@M41Npain In C `NULL` is the standard symbol for a null pointer. If, in your case, `stringhe` is equal to `NULL` after the first call to `malloc`, then the allocation failed and you should not continue using the pointer. – Some programmer dude Feb 15 '17 at 14:17

score 1 · Answer 2 · answered Feb 15 '17 at 14:20

1

There are some problems with your code:

This line:

char **stringhe = malloc((x) * sizeof(char));

Needs to be:

char **stringhe = malloc((x) * sizeof(char*)); /* or sizeof *stringhe */

As you need to allocate x char* pointers for stringhe.

Inside your first for loop, you are not adding +1 for null-terminator. It needs to be instead:
```
stringhe[i] = malloc(z+1); /* sizeof(char) = 1 */
```
You need to check return of malloc(). It can return NULL if unsuccessful. You can do this by simply checking if (ptr == NULL), then exit the program. It would unsafe to allow a failed malloc() to continue in the program.
for (int i = 0; i < t; i++) is comparing an int with size_t. This should be for (size_t i = 0; i < t; i++) instead.

answered Feb 15 '17 at 14:20

RoadRunner

25,803
6
42
75

I did not add +1 in `stringhe[i] = malloc((z)* sizeof(char));` because after the malloc i see with the the debugger, that the malloc function always allocates more space that is necessary, in fact then I added `stringhe[i][z]= 0;` to cut the rest – Amarildo Feb 15 '17 at 14:27
1

@M41Npain When allocating `char*` pointers, it is always safe to add that extra +1 when using `malloc()`. It may work now, but in the future, if you don't do this, and `malloc()` does not allocate enough space for the `\0`, then you will be accessing beyond the bounds of what was allocated. – RoadRunner Feb 15 '17 at 14:30
1

@M41Npain When dealing with heap allocation, its better to be safe then sorry. I'm glad I could help :). – RoadRunner Feb 15 '17 at 14:33

Deallocation of memory in arrays

2 Answers2