0

I'm working on making a C program that basically can take a sentence and count how many times each word appears in it. I've made a stripped down version that reproduces the issue exactly.

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

typedef struct testStrut{ 
    char * string; 
    int uses; 
}word; 

void grow(); 

int main(){
    int i; 
    int count = 1; 
    word ** words; 

    words = (word **) malloc(count * sizeof(word *)); 
    words[0] = (word *) malloc(sizeof(word)); 

    for(i = 0; i < 10; i++){
        printf("Re-looping i: %d \n", i); 
        printf("words[0]->string = %s \n", words[0]->string); 
        grow("TEST", words, &count); 
    }

    printf("Done."); 
    return 0; 
}

void grow(char * str, word ** words, int * count){
    word ** tmp; 

    tmp = realloc(words, (*count) * sizeof(word *)); 

    if(tmp){
        tmp[(*count)-1] = malloc(sizeof(word)); 
        tmp[(*count)-1]->string = malloc(strlen(str)+1); /*+1 for null terminator as pointed out */  
        strcpy(tmp[(*count)-1]->string, str); 
        tmp[(*count)-1]->uses = 1;     
        words = tmp; 
        (*count)++; 
    } else{
        printf("Failure to allocate. \n"); 
        exit(0); 
    }
    printf("Count: %d and word[0] %s \n", (*count), str); 
}

As well as the output from a run:

Re-looping i: 0
words[0]->string = (null)
Count: 2 and word[0] TEST
Re-looping i: 1
words[0]->string = TEST
Count: 3 and word[0] TEST
Re-looping i: 2
words[0]->string = TEST
Count: 4 and word[0] TEST
Re-looping i: 3
words[0]->string = TEST
Count: 5 and word[0] TEST            /*Prints it fine? */ 
Re-looping i: 4
Segmentation fault (core dumped)     /*Suddenly unable to print it? */ 

I'm not understanding why between ending the grow function and re-going through the loop the value of words[0]->str is suddenly lost. Is there something I'm missing?

[I do know that I should be freeing anything I malloc.I also realize my method prototype isn't the correct one but I just wanted to make a quick program that demonstrated my issue]

Nateb1121
  • 26
  • 5
  • 4
    Hint: What does `void f(int i) {i = 7;} int main() {int x = 5; f(x); printf("%d\n", x); return 0;}` print? Now change `int i` to `word ** words`, and `int x` to `word ** words`, and `7` to `realloc(...)` – user253751 Apr 07 '16 at 05:36
  • 1
    Another mistake is that you've made a wrong "string" allocation, length of a string is `strlen(str)+1` to take the NUL terminating char in account. – Jean-Baptiste Yunès Apr 07 '16 at 05:40
  • Your example prints 5 since i is passed by value, isn't when I pass word ** words it a pointer so any modifications would be reflected in main as well? Thanks Jean for catching that! My bad, I've edited to reflect that change. – Nateb1121 Apr 07 '16 at 05:45
  • It suddenly stops working when realloc decided to return a pointer to a new memory location. – zakinster Apr 07 '16 at 05:58
  • Is there any reasoning behind that? What's the proper way to fix this/do what I'm trying to do? – Nateb1121 Apr 07 '16 at 06:02
  • @Nateb1121 you have to properly use the pointer returned by realloc. As immbis said, you're not returning the value correctly. – zakinster Apr 07 '16 at 06:04
  • Do note that giving null pointer to `%s` has **undefined behaviour**, also on first iteration your `->string` pointer is **uninitialized**. – Antti Haapala -- Слава Україні Apr 07 '16 at 06:12
  • @AnttiHaapala Apologies, I wrote this program in a haste and didn't remember to correctly initialize that. I meant this to quickly serve as a demonstration. – Nateb1121 Apr 07 '16 at 06:14
  • [Don't cast the result of `malloc` in C](http://stackoverflow.com/q/605845/995714) – phuclv Apr 07 '16 at 06:24

2 Answers2

2

On the first iteration of the for loop the following line is accessing uninitialized memory.

printf("words[0]->string = %s \n", words[0]->string);

You have also declared

void grow();

but the actual signature ie

void grow(char * str, word ** words, int * count)

You first need to call grow before that line ie. You are also realloc and assuming that the pointer words in main points to the original pointer.

Try this. I've simplified a little bit...

#include <assert.h>
#include <stdio.h>
#include <stdlib.h> 
#include <string.h>
typedef struct testStrut{ 
  char * string; 
  int    uses; 
} word; 

void grow(const char *str, word *words, int count); 

int main(){
  int i;  
  word * words; 
  printf("sizeof word == %zu\n", sizeof(word));
  assert(sizeof(word) == 16);
  words    = malloc(sizeof(word)); 
  for(i = 0; i < 10; i++){
    printf("Re-looping i: %d \n", i); 
    grow("TEST", words, i); 
    printf("words[0]->string = %s \n", words[0].string); 
  }
  printf("Done."); 
  return 0;  
}
void grow(const char * str, word *words, int count){
  word ** tmp; 
  int idx = count - 1;
  printf("size == %zu\n", count * sizeof(word));
  tmp = realloc(words, count * sizeof(word)); 
  size_t str_len = strlen(str); 
  if(tmp != NULL) {
    tmp[idx]         = malloc(sizeof(word*)); 
    tmp[idx]->string = malloc(str_len + 1); 
    strcpy(tmp[idx]->string, str); 
    tmp[idx]->string[4] = '\0';
    tmp[idx]->uses = 1;
  } else{
    printf("Failure to allocate. \n");
    exit(0);
  }
  printf("Count: %d and word[0] %s \n", count, str);
}
Harry
  • 11,298
  • 1
  • 29
  • 43
  • I realize that now in my haste to make this, however, would that be causing the issue I'm experiencing? I understand I'm now using the correct method prototype, this is just a quick dirty example to demonstrate the issue. – Nateb1121 Apr 07 '16 at 05:46
  • count is incremented each time the grow method is executed by the (*count)++ line. – Nateb1121 Apr 07 '16 at 05:54
  • `void grow();` is allowed, it just means that the compiler won't validate the argument types for you. Fortunately OP used the right types in the call. Of course it is a good idea to use the prototype so that the compiler helps you out. – M.M Apr 07 '16 at 05:54
  • @Nateb1121 yes, `words[0]->string` is an uninitialized pointer, so it is quite likely that trying to print a string from where it points will cause a segfault – M.M Apr 07 '16 at 05:55
  • @M.M If so then why would it still successfully run through the loop 4 times before segfaulting? – Nateb1121 Apr 07 '16 at 06:00
  • maybe the random junk happened to be different the 5th time. who knows? – M.M Apr 07 '16 at 06:45
0

The problem is here :

words = tmp; 

This statement has no effect outside of the function.

Since realloc may (or may not) return a new pointer to another memory location, you code may work a few times before crashing.

You should use a word*** instead of the word** parameter or simply return the new pointer:

word** grow(char * str, word ** words, int * count){
    word ** tmp; 

    tmp = realloc(words, (*count) * sizeof(word *)); 

    if(tmp){
        tmp[(*count)-1] = malloc(sizeof(word)); 
        tmp[(*count)-1]->string = malloc(strlen(str)+1); /*+1 for null terminator as pointed out */  
        strcpy(tmp[(*count)-1]->string, str); 
        tmp[(*count)-1]->uses = 1;   
        (*count)++; 
    } else{
        printf("Failure to allocate. \n"); 
        exit(0); 
    }
    printf("Count: %d and word[0] %s \n", (*count), str); 
    return tmp;
}

And call it this way:

words = grow("TEST", words, &count); 
zakinster
  • 10,508
  • 1
  • 41
  • 52
  • What would be the proper way to assure that the value of word is overwritten to be the address of tmp? It seems I'm mistaken in my assumption that word ** words is passed as a pointer and that any modifications to it would be reflected outside the function... – Nateb1121 Apr 07 '16 at 06:04
  • @Nateb1121 You can simply return the new pointer as value instead of trying to update a function parameter. Otherwise a pointer to word** (i.e. word***) would be necessary. – zakinster Apr 07 '16 at 06:07
  • Oh, duh! Apologies for my line of questioning but if I'm understanding right because word ** words is a pointer I can change the contents of that pointer and have it reflected outside the grow() function, however I can NOT change the actual place where words points and expect it to have any effect outside grow()? – Nateb1121 Apr 07 '16 at 06:10
  • @Nateb1121 Yes, a pointer is a variable containing a value (an address to a memory location), you can change the data at that memory location (which would reflect outside a function), but if you change the value of the variable (the pointer) it will only affect the scope of the variable (here the function). – zakinster Apr 07 '16 at 06:14
  • Thank you! That did the trick. I feel silly for overlooking that! – Nateb1121 Apr 07 '16 at 06:16