Will there be any memory leak if I use malloc in this context

Question

int num_words = 0;
while ((c = fgetc(in_fp)) != EOF) {
    if (c == 32 || c == 10 || c == 13 || c == 46) {
        //32 is Space, 10 is LF, 13 is CR, 46 is period
        //todo: Add other kinds of punctuation
        num_words = num_words + 1;
    }
}                                   

char** words = (char**) malloc(num_words * sizeof(char*));
if (words == NULL) {
    printf("Memory allocation failed\n");
    return 1; //abort program
}

//Reset the input file pointer to the start of the file
rewind(in_fp);

//Allocate enough space for each word
int word_being_allocated = 0;
int word_size = 0;
int size;
while ((c = fgetc(in_fp)) != EOF) {
    if (c == 32 || c == 10 || c == 13 || c == 46) {
        //32 is Space, 10 is LF, 13 is CR, 46 is period
        size = (word_size + 1) * sizeof(char);
        words[word_being_allocated] = (char*) malloc(size);
        if (words[word_being_allocated] == NULL) {
            printf("Memory allocation failed\n");
            return 1;
        }
        word_being_allocated = word_being_allocated + 1;
        word_size = 0;
        continue;
    }
    word_size = word_size + 1;
}
for (int i = 0; i < num_words; i++) {
    free(words[i]);
}
free(words);

Will there be any memory leak, since I am using malloc twice. My question here is since I am already allocating memory for **words, when I am writing words[word_being_allocated] = (char*) malloc(size); isn't it again allocating.

Why do you [cast the result of `malloc()`](http://stackoverflow.com/q/605845/296974)? — glglgl, Oct 07 '16 at 15:17
This is not your real code, or a [MCVE]. e.g. you don't close your `while` or `if`.... — Lightness Races in Orbit, Oct 07 '16 at 15:17
This is not the complete code. I just added the partial code. Do you want me to put the complete code? — Phaneeth, Oct 07 '16 at 15:19
Complaints unrelated to actual question: 1) C and C++ are different languages, pick one, 2) don't cast the return value of `malloc` if this is C, 3) `sizeof(char)` is always `1` by definition, so multiplying by it is unnecessary (note that `sizeof(char *)` is something else). — Arkku, Oct 07 '16 at 15:20
We are also not shown the end of the loop, i.e., it is unknown if `word_being_allocated` is actually incremented somewhere and if it is checked at that time against `num_words` to avoid out of bounds access. Likewise we do not know if you ensure that all of `num_words` gets used, otherwise you may end up freeing uninitialized pointers. — Arkku, Oct 07 '16 at 15:22
Shouldn't the `free` loop depend on `word_being_allocated` since the input is not controlled by `num_words` but by the input data itself? — Weather Vane, Oct 07 '16 at 15:23
No, I want you to post your [MCVE], as instructed in the Help Centre's section on How To Ask. — Lightness Races in Orbit, Oct 07 '16 at 15:27
Sorry. I am new to stackoverflow and this is my very first question. I apologize and I have updated the code. Please let me know if you need anything else — Phaneeth, Oct 07 '16 at 15:30
If you reach `EOF` and `word_being_allocated < num_words` you will be trying to `free` memory that was never allocated. Never rely on input data being what you expect it should be. — Weather Vane, Oct 07 '16 at 15:30
"Famous Last Words" have been uttered by many. As already said you should free the the memory that *was* allocated, not what you think *should* have been be allocated — Weather Vane, Oct 07 '16 at 15:43

score 0 · Answer 1 · edited Oct 07 '16 at 15:17

0

As far as we can see, the code you provide is fine as far as you don't discard any malloc()ed memory. So at least you have the chance to be leak-free. Whether you really are depends on how you proceed after working with the data.

edited Oct 07 '16 at 15:17

Lightness Races in Orbit

378,754
76
643
1,055

answered Oct 07 '16 at 15:16

glglgl

89,107
13
149
217

Mr.C64 · Answer 2 · 2016-10-07T15:24:16.103

0

You don't have memory leaks as far as you properly free() every single memory block you allocate with malloc().

EDIT In this code:

while ((c = fgetc(in_fp)) != EOF) {
    if (c == 32 || c == 10 || c == 13 || c == 46) {
        //32 is Space, 10 is LF, 13 is CR, 46 is period
        size = (word_size + 1) * sizeof(char);
        words[word_being_allocated] = (char*) malloc(size);

You don't seem to update word_being_allocated, so you may overwrite the same pointer slot in the words array, and in this case leaking memory (as you don't free the previous allocated pointer).

When you update word_being_allocated properly, make sure to not overflow the bounds of the words pointer array.

edited Oct 07 '16 at 15:24

answered Oct 07 '16 at 15:17

Mr.C64

41,637
14
86
162

char** words = (char**) malloc(num_words * sizeof(char*)); words[word_being_allocated] = (char*) malloc(size) I am worried about this part of the code. – Phaneeth Oct 07 '16 at 15:17
I have updated the partial code. I would appreciate if you could take a look – Phaneeth Oct 07 '16 at 15:29

Will there be any memory leak if I use malloc in this context

2 Answers2