How can I ensure that I am not missing something in malloc and free?

Question

char **test() {
    char **res = (char **)malloc(sizeof(char *) * 5);
    for (int cur = 0; cur < 3; cur++) {
        char *str = (char *)malloc(10);
        strcpy(str, "Maneger");
        res[cur] = (char *)malloc(strlen(str));
        strcpy(res[cur], str);
        free(str);
    }
    res[3] = NULL;
    return res;
}

int main(int argc, char *argv[]) {
    char **li = test();
    //some code
    free(li);
    return 1;
}

What is wrong with free() in above code. Is it correct?

if you are using C++ don't use `malloc`. If you are using C, don't tag this as C++ — Fureeish, Aug 19 '17 at 12:50
You just forgot to free some pointers. (And should cast malloc return pointer) — kocica, Aug 19 '17 at 12:51
Each `malloc` call should have a matching `free` call. You have three calls to `malloc` but only two to `free`. One of the `malloc`/`free` pairs you have is not needed by the way. — Some programmer dude, Aug 19 '17 at 12:51
They say [you shouldn't cast the result of `malloc()` in C](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — MikeCAT, Aug 19 '17 at 12:51
Allocating `strlen(str)` bytes is not enough for copying `str`: you must allocate one more element for the terminating null character. — MikeCAT, Aug 19 '17 at 12:52

score 1 · Answer 1 · answered Aug 19 '17 at 12:54

1

You forgot to allocate space for the terminating null character.

The line

res[cur]=(char *)malloc(strlen(str));

should be

res[cur]=(char *)malloc(strlen(str) + 1);

Also checking for return values ofr malloc() not to be NULL should be added.

answered Aug 19 '17 at 12:54

MikeCAT

73,922
11
45
70

score 1 · Answer 2 · answered Aug 19 '17 at 13:21

The free in main is not correct. It only frees the memory allocated to li pointer not the memory allocated to the array of pointer associated with it.

After freeing the li pointer the memory allocated to the array of pointer associated with it will become inaccessible which in wrong.

The correct way to free all memory is following :

free(li[0]);
free(li[1]);
free(li[2]);

And after this statements there should be:

free(li);

Hope it will help !!

score 0 · Answer 3 · answered Aug 19 '17 at 14:40

There are multiple problems in your code:

why do you allocate space for 5 pointers but only use 4?
why do you allocate 10 bytes for str, copy a string to it, duplicate that and free str for every iteration?
you do not allocate space for the null terminator in the copy of "Maneger".
you do not free the strings in li.
you do not check for potential malloc failures.

Here is a corrected version:

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

void free_test(char **a) {
    if (a) {
        for (int i = 0; a[i]; i++) {
            free(a[i]);
        }
        free(a);
    }
}

char **test(void) {
    char **res = malloc(sizeof(*res) * 4);
    if (res) {
        int cur;
        for (cur = 0; cur < 3; cur++) {
            res[cur] = strdup("Maneger");
            if (res[cur] == NULL) {
                free_test(res);
                return NULL;
            }
        }
        res[cur] = NULL;
    }
    return res;
}

int main(int argc, char *argv[]) {
    char **li = test();
    if (li == NULL)
        return 1;
    //some code
    free_test(li);
    return 0;
}

strdup() is a much simpler and safer way to duplicate strings. It is not part of Standard C, but supported on Posix and other systems. If it is not available on your system, it can be redefined this way:

char *strdup(const char *s) {
    size_t len = strlen(s);
    char *p = malloc(len + 1);
    if (p != NULL) {
        memcpy(p, s, len + 1);
    }
    return p;
}

How can I ensure that I am not missing something in malloc and free?

3 Answers3