malloc 2d array

What exactly am I doing wrong here?
I need to dynamically allocate just enough space for each string on each line.

My teacher said to start with dynamically allocating enough space for 100 lines which I think I did right. Then I assumed next while I'm reading the file I'd take each line and use malloc with enough memory just to store each string.

With my code right now, I run it on Dev-C and it's printing out 104 lines. I run it on gcc and it's printing out 120 lines. I would think since I made memory for 100 lines up top that's all it should print out? (the file I'm reading has about 600 lines but I haven't gotten as far as to keep reallocating more memory)

Code:

---

int main(){
    FILE *fp;
    char buffer[50];
    char **words;
    int i=0;
   
  words = malloc(100 * sizeof(char *) );
   
    if ( (fp = fopen("words.txt", "r" )) == NULL )
    {
    printf("Couldn't open file\n");
    exit(1);
    }
   
    while( fgets(buffer, sizeof(buffer), fp))
    {
          words[i] = malloc(strlen(buffer)*sizeof(char));
          strtok(buffer, " \n");
          strcpy(words[i], buffer);
          printf("%d: %s\n", i+1, words[i]);
          i++;
    }
getchar();
return 0;
}

---

Nope, there's no way for it to know how much you malloc'ed. Your while loop never stops when i >= 100, so you overflow the array of 100 strings. The reason it's 104 lines in Dev-C++ and 120 in GCC is pure randomness. You're in the territory of undefined behavior.

I realize this may not be finished code, but some other issues I noticed:

• You need to free your memory when you're done. It's a good habit to be in, even if you're just calling exit(1) later.
• You also need to close your file when you're done.
• You don't need sizeof(char) in your malloc call. The standard guarantees it will always be 1. But you do need the sizeof for the malloc on line 7.
• Are you sure you want a space in your strtok tokens? That will only get the first word from each line, unless you put your strtok/malloc/strcpy into a loop for each line.
• If after calling fgets, buffer does not contain a space or a new line, you have a buffer overflow. Your code doesn't guarantee room for the null. Just add a +1 to the amount you malloc.

Code:

---

#define MAXLINESIZE 50
#define MAXLINES 100
int main(){
  FILE *fp;
  char buffer[MAXLINESIZE];
  char *words[MAXLINES]; /* define enough space for max. 100 StringPOINTERS */
  int i=0;
   
  if ( (fp = fopen("words.txt", "r" )) == NULL )
  {
    perror("words.txt");
    exit(1);
  }
   
  while( i<MAXLINES && fgets(buffer, MAXLINESIZE, fp)) /* no lines-overflow */
  {
    strtok(buffer, "\n"); /* strtok is not multithread-safe */
    words[i] = malloc(strlen(buffer)+1); /* +1 for terminating '\0' */
    strcpy(words[i], buffer);
    printf("%d: %s\n", i+1, words[i]);
    i++;
  }
  fclose(fp);
  while( i-- ) free(words[i]); /* free all mallocs */
 
  getchar();
  return 0;
}

---

Looks much better. Does it work?

Ok, I just put the fclose in there and am going to write a function for freeing it all.
So I still need sizeof (char *) or no? I understand why I don't need sizeof(char). Wasn't really thinking about that at first.

And I see your point on the space. I'm just trying to get rid of the newline. He says I need to allocate enough space for the string on each line which includes the ("\0") terminating NULL. So just adding the +1 to the malloc should guarantee that right?

and to make things easier the format of the file I'm reading is always going to be something like this...

cat
dog
apple
horse

I have a question though. So initially I malloced words to be basically have 100 spots in the array right? And the second malloc is supposed to be giving memory just enough for the string on that line. Hard for me to explain, I have a drawing that will explain it a little better I think. But I realize the the while loop keeps going and I keep malloc more memory for that part of the array. Because it is a 2d array right? So I'm not understanding how come after it reaches the first 100 lines theres no error basically saying hey we don't have enough room in the array for this. (that's what's supposed to happen. In the long run I need to double the memory each time it reaches the limit of memory allocated).

I'm attaching a picture to hopefully explain it better than I can.
Well I tried to attach a picture and it kept saying it was too large. Resized it and couldn't get it to a good enough size. So here's my photobucket link for reference of the picture.

http://i222.photobucket.com/albums/d...ecar/008-1.jpg

Thanks Billy,
I'm reading through it now.

Only thing that I see so far is that one of the requirements is this...

"Read the data file once, storing each word in its own dynamically allocated array of typechar. To keep track of all of the words, you will create a dynamically allocated array of type
char *. This means that you will do something like this


char **words;


instead of


char * words[];"

So I couldn't do your char* words[MAXLINES] part
reading the rest now

Thanks for your help guys. Billy's code works. Gonna mess with it some to fit my requirements and run with it. I'll let you know if I need more help. Thanks a lot for the help. I see why you were saying it was going over the amount of lines now

@mgracecar:
Yes, you still need sizeof(char *).
C doesn't do any kind of array bounds checking, whether you malloc or not. It will let you write virtually anywhere in memory, it just might cause massive problems. It's your responsibility to keep track of how much you allocate and to not write outside of that space. Read this: Question 7.27. Also, this might be useful: Question 6.16.

You should declare a variable, called something like max_words, which will keep track of how many char * you allocated and can use in your words "array". Start with it at 100 or whatever, and double it each time you realloc. Use that in your while loop to avoid overflowing words.

Note that when it comes time to use realloc, you must remember to use a temporary pointer. Otherwise, if realloc fails, you won't be able to access the data nor will you be able to free it when you're done.

Quote:

---

I run it on Dev-C and it's printing out 104 lines. I run it on gcc and it's printing out 120 lines.

---

Dev-Cpp is presumably using gcc too. It doesn't have it's own compiler, although you could configure it to use something else. Look under Tools/Compiler Options/Programs to see what it's using.

Both your original code and Billy Theodore Kidstein's code are strange in their use of strtok. It is usually used thus:

Code:

---

char *p = strtok(buffer, " \n"); // point to first word in buffer
while (p) {
    words[i] = malloc(strlen(p) + 1); // no need for sizeof(char)
    strcpy(words[i++], p);
    p = strtok(NULL, " \n"); // point to next word in buffer
}

---

This works here for the first part of the code..
How's it looks guys??

*note* I put it all in a function to create more organization for later.

Ok cool ooga I see what you did, with that. Not as familiar with that way right now. But definitely seems to make it easier on the coder in not having to write as much.

Code:

---


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




char **getWords(char *wordsFile);


int main()
{
    char *wordsFile = "words.txt";
    char **words;
   
    words = getWords( wordsFile );
getchar();
return 0;
}


char **getWords(char *wordsFile)
{
    FILE *fp;
    char buffer[20];
    char **words, **temp;
    int i=0;
    int n = 100;
   
    words = malloc(100 * sizeof(char *) );
   
    if ( (fp = fopen("words.txt", "r" )) == NULL )
    {
    printf("Couldn't open file\n");
    exit(1);
    }
   
    while( fgets(buffer, sizeof(buffer), fp))
    {
          strtok(buffer, "\n");
          words[i] = malloc(strlen(buffer)+1);
          strcpy(words[i], buffer);
          printf("%d: %s\n", i+1, words[i]);
         
          if ( (i+1) == n )
          {
              n = n*2;
              temp= realloc(words, n*sizeof(char*));
              if(temp != NULL)
                      words = temp;
              else
              {
                  printf("unable to reallocate\n");
                  exit(1);
              }
              printf("newline here\n");
          }   
          i++;
    }       
fclose(fp);
   
return words;   
}

---

The best way to read 'words' (separated by whitespaces) from a stream is fscanf, and to implement a dynamic array you can use realloc like:

Code:

---

int main(){
  FILE *fp;
  char buffer[50];
  char **words=0;
  int i=0;
   
  if ( (fp = fopen("words.txt", "r" )) == NULL )
  {
    perror("words.txt");
    exit(1);
  }
   
  while( 1==fscanf(fp,"%49s",buffer) )
  {
    words=realloc(words,++i * sizeof*words);
    words[i-1]=malloc(strlen(buffer)+1);
    strcpy(words[i-1],buffer);
    printf("%d: %s\n", i, words[i-1]);
  }
  fclose(fp);
  while( i-- ) free(words[i]); /* free all mallocs */
  free(words); /* free realloc */
 
  getchar();
  return 0;
}

---

Looks pretty good, I don't see any obvious bugs. Only a few minor suggestions:
1. On line 30, use n instead of 100: malloc(n * sizeor(char *))
2. Don't call exit(1). Instead, return NULL to main and let main handle the error, in case you need to do any other clean up. It's generally poor practice to call exit from anywhere, especially in a library. There's no control over how to handle the error.
3. If you encounter an error in getWords, before you return NULL you need to free all the memory you've allocated. That would be all the words[i]'s that you've malloc'ed so far, plus words itself.

Quote:

---

Originally Posted by BillyTKid

The best way to read 'words' (separated by whitespaces) from a stream is fscanf, and to implement a dynamic array you can use realloc like:

---

He's not actually reading words separated by whitespace. He's reading a whole line at a time. He's just using strtok to trim the newline that fgets is putting into buffer.

Quote:

---

Originally Posted by anduril462

He's not actually reading words separated by whitespace. He's reading a whole line at a time. He's just using strtok to trim the newline that fgets is putting into buffer.

---

His original code had a space and a newline in the character list to strtok, so it seemed that he was doing more than just nulling the newline. However his most recent code just has newline, so I guess you're right.