dynamic memory and fgets

Question

Hi to all stackoverflow users. I am trying to build a simple (as an exercise) code that will read from a file and will store the words from a file in an dynamically allocated array. I think I am mallocing wrong. Does anyone see what I am doing wrong?

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

#define ARRSIZE 10

int main(){
    char * myArray = malloc(ARRSIZE*sizeof(char*));
    FILE * p1File;
    char mystring1 [100];
    char word [100];
    int j = 0;
    p1File = fopen ("my1file.txt","r");
    if (p1File == NULL) perror ("Error opening file");
    else{
        while(fgets(mystring1, 100, p1File)){
            int nuRead = sscanf(mystring1, "%s", word);\
            printf("lepo ani magia\n\n");
            if (nuRead > 0){
                strncpy (*myArray[j], mystring1, 100);
                //*myArray[j] = mystring1;
            }
            j += 1;
        } 
    }
}

///////////////////////////////////

my text file is

this
will
probably
work
but
I
am

Answer 1

You're not allocating space for your strings, just the array of strings. myArray[j] is just an uninitialized pointer. Instead, allocate space for each string in myArray like so:

char *myArray[ARRSIZE]; // No reason for this to be dynamic.
// ...
if (nuRead > 0)
{
    myArray[j] = malloc((strnlen(mystring, 100) + 1) * sizeof(char));
    strncpy (myArray[j], mystring1, nuRead + 1);
}

As user411313 pointed out, sscanf doesn't return the number of characters matched, but the number of input items matched. Use strnlen (or strlen if you don't have strnlen) to get the size of the string (and don't forget to add 1 for the null terminator).

Answer 2

For this task I would first define a data structure that holds the words, like this:

struct wordlist {
    char **words; /* the actual words */
    size_t size; /* the number of words in the list */
    size_t capacity; /* the number of words that would fit in the list */
};
typedef struct wordlist wordlist;

Then I would define some functions to operate on them. This is to keep the code in main short and readable. The functions are:

void *
malloc_or_fail(size_t size)
{
  void *result = malloc(size);
  if (result == NULL) {
    perror("malloc");
    exit(EXIT_FAILURE);
  }
  return result;
}

/* Creates a newly allocated copy of the given string. Later changes
 * to the given string will not have any effect on the returned string.
 */
char *
str_new(const char *str) {
  size_t len = strlen(str);
  char *result = malloc_or_fail(len + 1);
  memcpy(result, str, len + 1);
  return result;
}

/* Adds a copy of the given string to the word list. Later changes
 * to the given string have no effect on the word in the word list.
 */
void
wordlist_add(wordlist *wl, const char *word)
{
  if (wl->size == wl->capacity) {
    /* TODO: resize the wordlist */
  }
  assert(wl->size < wl->capacity);
  wl->words[wl->size++] = str_new(word);
}

/* Creates a new word list that can hold 10 words before it will be
 * resized for the first time.
 */
wordlist *
wordlist_new(void)
{
  wordlist *result = malloc_or_fail(sizeof wordlist);
  result->size = 0;
  result->capacity = 10;
  result->words = malloc_or_fail(result->capacity * sizeof result->words[0]);
  return result;
}

Using these functions it shouldn't be difficult to complete the original task.

Answer 3

char * myArray = malloc(ARRSIZE*sizeof(char*));

You have allocated a place to store ten string pointers. But you have not allocated any space to copy the characters into persistent strings.

If you want to set up that storage at the beginning, you could do

#define MAX_STR_SIZE 100

char * myArray = malloc(ARRSIZE*sizeof(char*));
if (!myArray) exit(1);
for (j=0; j<ARRSIZE; j++) {
    myArray[j] = malloc(MAX_STR_SIZE);
    if (!myArray[j]) exit(1);
}

Or, probably better, you can allocate each string as needed. Instead of strncpy, use strdup (which is like doing a malloc then a strcpy):

    myArray[j] = strdup(mystring1);

Answer 4

If you only need to handle up to 10 lines of text then I'd do it more like this:

char *myArray[ARRSIZE];
...
if (nuRead > 0) {
  myArray[j++] = strdup(mystring1);
}
...

What's happening is that this code allocates and copies in one go (using strdup, rather than malloc followed by strcpy).