proper memory allocation for strings

Question

So I've had this problem I've been trying to solve for about 8 hours now... I've given up my search for an answer without help. I've tried using realloc() and malloc() respectively, so any input would be great!

The purpose of this being in C is to allow for the creation of a 'map', I will later be using ncurses to create the map.

the input from the file is as follows

10X16 de4 dw9 ds8 g8,7 m3,4 h6,5 p2,2 
6X20 dn5 ds4 W4,3 e2,12 M1,1
10X13 ds3 dw9
10X12
5X4
6x12

Here is the code:

char *importLevel()
{
    FILE *fPointer; 
    fPointer = fopen("Level", "r"); //Opens text file to read
    char* rooms[150];// set up for memory allocation
    char commands[150];// set up for pulling data from read file

    while (!feof(fPointer))
    {
        fgets(commands,150, fPointer); // this takes each line from the file
    }

    *rooms = (char *) malloc(150 * sizeof(char)); //  memory allocation
    for (int i = 0; i < 150; i++)
    {
        if (rooms[i] != NULL)
        {
            *rooms[i] = commands[i]; // supposed to give rooms the string
        }
    }

    fclose(fPointer);// close file

    return *rooms; // return pointer
 }

I hope I'm not as stupid as I feel right now! Thanks :)

edit: I AM as stupid as I felt right then

Answer 1

There are quite a few things to address here.

while (!feof(fPointer))
{
    fgets(commands,150, fPointer); // this takes each line from the file
}

This is going to overwrite the data in commands each time through the loop. When the loop exits you will have read and discarded all the data except for the last line. You'll want to either use a 2-d array, or more likely, store the data into rooms as you read it. The second way is faster and uses less memory.

*rooms = (char *) malloc(150 * sizeof(char));

This sort of looks like you're trying to create a 2-d array. Instead you'll want to do something like this:

for (int ii = 0; ii < 150; ++ii)
  rooms[ii] = malloc(150 * sizeof(char));

Note that this malloc doesn't initialize the memory. So your check

if (rooms[i] != NULL)

Is going to give you undefined results. The contents of rooms[i] is undefined. If you want to initialize the array to all zeros, try using memset.

Then:

*rooms[i] = commands[i];

Isn't going to copy over the data from commands, rather it will only copy the first character from commands. To copy the whole string, you'll want to use strcpy or strncpy to avoid potential buffer overflow problems. memcpy is also an option to copy some number of bytes instead of null-terminated C-strings.

Lastly, returning *rooms is an error waiting to happen. You'd be better served passing rooms in as a parameter and allocating into that. See Allocate memory 2d array in function C for how to do that.

Answer 2

Others have pointed out some of the changes you'd need to make. So, take a look at what they've said and compare your version against the one below. This should help.

Here's a corrected version of your program. I had to guess at some of your intent, based upon your code and data [please pardon the gratuitous style cleanup]:

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

char **
importLevel()
{
    FILE *fPointer;
    char commands[150];                 // file line buffer
    int roomcnt = 0;
    char *cp;
    char *bp;
    char **rooms = NULL;                // set up for memory allocation

    fPointer = fopen("Level", "r");     // Opens text file to read
    if (fPointer == NULL) {
        printf("importLevel: unable to open file -- %s\n",strerror(errno));
        exit(1);
    }

    while (1) {
        // this takes each line from the file
        cp = fgets(commands, sizeof(commands), fPointer);
        if (cp == NULL)
            break;

        // setup buffer for strtok
        bp = commands;

        // parse all words on line
        while (1) {
            // NOTE: the first assumes you want "g8,7" within a room
            // the second assumes you want "g8,7" as two separate rooms
#if 1
            cp = strtok(bp," \n");
#else
            cp = strtok(bp," ,\n");
#endif

            // strtok wants this on 2nd and subsequent loops for this line
            bp = NULL;

            // bug out if no more words on this line
            if (cp == NULL)
                break;

            // increase room list size (allow space for terminator)
            // NOTE: rooms will be preserved when we return from this function
            rooms = realloc(rooms,sizeof(char *) * (roomcnt + 2));

            // NOTE: cp is pointing to something in our stack frame that
            // will go away when we return or when we read the next line
            // so we must preserve it in the heap now
            cp = strdup(cp);

            // add the contents of the room
            rooms[roomcnt] = cp;

            // advance count of number of rooms
            ++roomcnt;
        }
    }

    // add terminator to list
    if (rooms != NULL)
        rooms[roomcnt] = NULL;

    fclose(fPointer);                   // close file

    return rooms;                       // return pointer
}

---

P.S. Don't feel bad. No matter how experienced a programmer is, we all make "dumb" mistakes. And, we make them every day. Welcome to the club!