realloc, for string array in C

Question

Is there a correct way to use realloc, for when you want to add words of variable size to a string array? I am getting a segmentation fault. Please show me what's wrong

// This function puts every word found in a text file, to a String array, **words
char **concordance(char *textfilename, int *nwords){
    FILE * fp;
    char *fileName = strdup(textfilename);
    fp = fopen(fileName, "r");
    if(fp == NULL) {
        perror("fopen");
        exit(1);
    }
    char **words = malloc(sizeof(char));
    // char **words = NULL

    char line[BUFSIZ];
    while(fgets(line, sizeof(line), fp) != NULL){
        char *word = strdup(line);
        word = strtok(word, " ");
        do{
            words = realloc(words, (*nwords+1) * sizeof(char(*)));
            words[*nwords] = word;
        } while((word = strtok(NULL, " ")) != NULL);
    }
    return words;
}


int main(int argc, const char * argv[]) {
    int *nwords = malloc(sizeof(int));
    nwords = 0;
    concordance("test.txt", nwords);
}

Have you used a debugger (gdb or valgrind) to check your program?! — hellow
– hellow, Commented Nov 5, 2018 at 12:33
char **words should be allocated to size of pointer and to the maximum number of pointers you want to store, e.g. char **words = malloc(sizeof(char *)*1000); for a maximum of 1000 pointers. — Antonio Ragagnin
– Antonio Ragagnin, Commented Nov 5, 2018 at 12:38
You really shouldn’t be using strtok() unless you really know what you’re doing, because it modifies the string you’re scanning replacing the delimiter with a null. — Marichyasana
– Marichyasana, Commented Nov 5, 2018 at 13:08

ram914 · Accepted Answer · 2018-11-06 10:10:32Z

1

You seem to initialize nwords to 0, in a wrong way. As you have declared it as a pointer, you can not access it directly. instead, you should use the de-reference operator *.

make the following change in the main function

*nwords = 0; instead of nwords = 0;

nwords = 0 modifies the location to which nwords is pointing to, to the location with address 0, to which you have no access and can not assign.

WARNING:

It is better not to perform realloc on the same pointer, it will make the pointing location NULL if the realloc fails, leading to the loss of the previously existing data. Instead, as @David suggests, you could use a temp variable to realloc memory and then, check if it is not NULL and then assign its contents to the words pointer.

    //your code
    char *tmp = realloc(words, /* new size*/);
    if(tmp != NULL)
        words = tmp;
    // your code

while using realloc you usually use it to allocate a block of data, not for a single location.

edited Nov 6, 2018 at 10:10

answered Nov 5, 2018 at 12:54

ram914

3314 silver badges21 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

David C. Rankin Over a year ago

See comment under other answer about "Never realloc with the pointer itself....". Always good to point out potential errors.

Antonio Ragagnin · Accepted Answer · 2018-11-05 13:23:48Z

0

When you initiate the value of nwords, you were overwriting its pointer address, not its value.

Additionally, as the commenter says, the line char **words = malloc(sizeof(char)); is not correct. But you always re-allocate the variable words so the code still works as expected. To make it super safe you should change it to char **words = malloc(sizeof(char*));

I use the line *nwords = 0; and now it works as expected.

#define BUFSIZ 1000
#include<stdio.h>
// This function puts every word found in a text file, to a String array, **words
char **concordance(char *textfilename, int *nwords){
  FILE * fp;
  char *fileName = strdup(textfilename);
  fp = fopen(fileName, "r");
  if(fp == NULL) {
    perror("fopen");
    exit(1);
  }
  char **words = malloc(sizeof(char));
  // char **words = NULL

  char line[BUFSIZ];
  while(fgets(line, sizeof(line), fp) != NULL){
    char *word = strdup(line);
    word = strtok(word, " ");
    printf("word='%s'\n",word);
    do{
      *nwords=*nwords+1;
      printf("nwords=%d\n",*nwords);
      words = realloc(words, (*nwords+1) * sizeof(char(*)));
      words[*nwords] = word;
    } while((word = strtok(NULL, " ")) != NULL);
  }
  return words;
}


int main(int argc, const char * argv[]) {
  int *nwords = malloc(sizeof(int));
  *nwords = 0;
  concordance("test.txt", nwords);
}

edited Nov 5, 2018 at 13:23

answered Nov 5, 2018 at 12:47

Antonio Ragagnin

2,3174 gold badges27 silver badges41 bronze badges

3 Comments

Lundin Over a year ago

char **words = malloc(sizeof(char)); can impossibly work as expected.

Antonio Ragagnin Over a year ago

it just gives a pointer to a singe byte. Then the code always reallocates it with the right amount of words anyway. It's not philosophically right, but works.

David C. Rankin Over a year ago

Never realloc with the pointer itself. If realloc fails (and it does) it returns NULL overwriting your address preventing the original block from ever being freed creating a memory leak. Use a temp pointer, e.g. void *tmp = realloc (words, (nwords + 1) * sizeof *words); then validate tmp != NULL. (and note reallocating by 1-pointer each iteration is horribly inefficient, better to allocate multiple pointers in blocks and keep track of the number used and realloc when you run out)

Collectives™ on Stack Overflow

realloc, for string array in C

2 Answers 2

1 Comment

3 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

3 Comments

Your Answer

Sign up or log in

Post as a guest

Related