Cannot get realloc() to work

Question

FILE *file;
file = fopen(argv[1], "r");
char *match = argv[2];
if (file == NULL) {
    printf("File does not exist\n");
    return EXIT_FAILURE;
}
int numWords = 0, memLimit = 20;
char** words = (char**) calloc(memLimit, sizeof(char));
printf("Allocated initial array of 20 character pointers.\n");
char string[20];

while (fscanf(file, "%[a-zA-Z]%*[^a-zA-Z]", string) != EOF) {
    words[numWords] = malloc(strlen(string) + 1 * sizeof(char));
    strcpy(words[numWords], string);
    printf("Words: %s\n", words[numWords]);
    numWords++;     /*keep track of indexes, to realloc*/ 
    if (numWords == memLimit) {
        memLimit = 2 * memLimit;
        words = (char**) realloc(words, memLimit * sizeof(char*));   /*Fails here*/
        printf("Reallocated array of %d character pointers.\n", memLimit);
    }

}

Code should open and read a file containing words with punctuation, spaces etc and store in a string, but after 20 tries it throws an error, and I can't seem to get realloc() to work here, which I'm expecting to be the problem. The array is dynamically allocated 20 char pointers, at which when limit is reached, it should realloc by double. How can I get around this?

`sizeof` on the first `words` allocation should be `sizeof(char*)`. Is it just a typo? — Enzo Ferber, Sep 05 '15 at 03:44
You have a problem in your first `calloc`, you have `sizeof(char)` and not `sizeof(char*)` — Sami Kuhmonen, Sep 05 '15 at 03:45
@user3128077 that's why you **don't** do `sizeof(char *)` explicitly. Instead, write `sizeof words[0]`. That will yield the correct size, no matter what type `words` is. And don't cast the return value of `malloc()`. — The Paramagnetic Croissant, Sep 05 '15 at 05:34

score 3 · Answer 1 · edited May 23 '17 at 12:14

3

Two notes. First, you shouldn't ever cast the return value of calloc/malloc/realloc. See this for more information.

Second, as others have pointed out in comments, the first calloc statement uses sizeof(char) and not sizeof(char*) like it should.

edited May 23 '17 at 12:14

Community

1
1

answered Sep 05 '15 at 03:49

N8Stewart

45
5

perhaps you should mention than for defensive programming purposes, one should use `sizeof array[0]` instead of `sizeof(explicit type)`. – The Paramagnetic Croissant Sep 05 '15 at 05:34

score 2 · Answer 2 · edited May 23 '17 at 11:44

words is a pointer to a pointer. The idea is to allocate an array of pointers.
The below is wrong as it allocates for memLimit characters rather than memLimit pointers.
This is the main issue

char** words = (char**) calloc(memLimit, sizeof(char)); // bad

So use an easy idiom: allocate memLimit groups of whatever words points to. It is easier to write, read and maintain.

char** words = calloc(memLimit, sizeof *words);

Avoid the while (scanf() != EOF) hole. Recall that various results can come from scanf() family. It returns the count of successfully scanned fields or EOF. That is typically 1 of at least 3 options. So do not test for one result you do not want, test for the one result you do want.

// while (fscanf(file, "%[a-zA-Z]%*[^a-zA-Z]", string) != EOF) {
while (fscanf(file, "%[a-zA-Z]%*[^a-zA-Z]", string) == 1) {

The above example may not every return 0, but the below easily could.

int d;
while (fscanf(file, "%d", &d) == 1) {

@Enzo Ferber rightly suggests using "%s". Further recommend to follow the above idiom and restrict input width to 1 less than the size of the buffer.

char string[20];
while (fscanf(file, "%19s", string) == 1) {

Suggest the habit of checking allocation result.

    // better to use `size_t` rather than `int `for array sizes.
    size_t newLimit = 2u * memLimit;
    char** newptr = realloc(words, newLimit * sizeof *newptr);
    if (newptr == NULL) {
       puts("Out-of-memory");
       // Code still can use old `words` pointer of size `memLimit * sizeof *words`
       return -1;
    }
    memLimit = newLimit;
    words = newptr;
  }

Thank you for the detailed explanation. When I was referencing pages here, casting char was what i generally saw, but I"ll reference these idioms next time. I also previously used "%s" however, in my question, I meant to not include punctuation in words, while delimiting whitespaces,"\n", and "\t", So that's why I used the character regex. Is there a better way of doing this? — user3128077, Sep 05 '15 at 16:00
@user3128077 1) "casting char" is not much of an issue, but note the `sizeof *words` in `calloc(memLimit, sizeof *words);`. 2) In C, `a-zA-Z` is the scanset 3) There are typically 256 different charterers, ~32 punctuation and 6+ white-spaces, not to mention digits, control characters and maybe others. So again: specify what you want, not what you do not want. Using `"%[a-zA-Z]%*[^a-zA-Z]"` is OK unless the first character is not a letter. Better to use `while (fscanf(file, "%*[^a-zA-Z]"), fscanf(file, "%19[a-zA-Z]", string) == 1)` — chux - Reinstate Monica, Sep 05 '15 at 16:43

score 1 · Answer 3 · answered Sep 05 '15 at 03:52

Errors

Don't cast malloc/calloc returns. There's not need for it.
Your first sizeof is wrong. It should be sizeof(char*)
That scanf() format string. %s does the job just fine.

Code

The following code worked for me (printed one word per line):

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

int main(int argc, char *argv[])
{
    FILE *file;
    file = fopen(argv[1], "r");
    char *match = argv[2];
    if (file == NULL) {
        printf("File does not exist\n");
        return EXIT_FAILURE;
    }
    int numWords = 0, memLimit = 20;
    char **words = calloc(memLimit, sizeof(char*));
    printf("Allocated initial array of 20 character pointers.\n");
    char string[20];

    while (fscanf(file, "%s", string) != EOF) {
        words[numWords] =
            malloc(strlen(string) + 1 * sizeof(char));
        strcpy(words[numWords], string);
        printf("Words: %s\n", words[numWords]);
        numWords++; /*keep track of indexes, to realloc */
        if (numWords == memLimit) {
            memLimit = 2 * memLimit;
            words = realloc(words, memLimit * sizeof(char *));  
            printf
                ("Reallocated array of %d character pointers.\n",
                 memLimit);
        }

    }
}

Called with ./realloc realloc.c

Hope it helps.

score 1 · Answer 4 · answered Sep 05 '15 at 05:06

1

Your first allocation is the problem. You allocate 20 chars and treat them as 20 char pointers. You overrun the allocated buffer and corrupt your memory.

The second allocation fails because the heap is corrupted.

answered Sep 05 '15 at 05:06

zmbq

38,013
14
101
171

Cannot get realloc() to work

4 Answers4

Errors

Code