newbie: need help using malloc in a loop

Hi all. :)

Here are my objectives:

1. Redirect a list of files in a directory into a text file.
2. Open the text file for reading.
3. Put each line of the file into an array of pointers, as strings.
4. Print the array of pointers(strings).

I need to allocate memory to an array of pointers, but I can't get the logic(sequence) right. The program compiles, but crashes.

Code:

---

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

#define NUMBEROFFILES 1500
#define FILENAMESIZE 40

int main( void )
{
    /* Array of pointers to store filenames */

    char *files[NUMBEROFFILES];
    int counter;
    FILE *openfile;
    char *string;

    /* Get a list of files in the directory "testwebsite", and redirect the
    output to a file called "directorylisting.txt". */

    system("dir c:\\testwebsite\\*.html > directorylisting.txt");

    /* Test to see that the file "directorylisting.txt" can be opened for reading. */

    if( (openfile = fopen("directorylisting.txt", "rt")) == NULL)
    {
        perror("\ndirectorylisting.txt");
        exit( EXIT_FAILURE );
    }

    counter = 0;

    while( fgets(files[counter], BUFSIZ, openfile) != NULL)
    {
        files[counter] = (char *)malloc( BUFSIZ * sizeof(char));  /* Try to allocate memory for string */

        if( files[counter] == NULL )
        {
            perror("\nMemory allocation error");
            exit( EXIT_FAILURE );
        }

        counter++;
    }

    fclose(openfile);

    /* Print the strings in the array of pointers */

    for( counter = 0; counter < NUMBEROFFILES; counter++)
        printf("\nFilename %d: %s", counter, files[counter]);

    return 0;
}

---

I know I am allocating the memory after it is needed, not before, but I just can't seem to achieve it. Do I need use a for() loop nested within the while() loop, or vice versa?

I've also tried:

Code:

---

while( (fgets(files[counter], BUFSIZ, openfile) != NULL) && (files[counter] = (char *)malloc( BUFSIZ * sizeof(char) != NULL )))

---

but it also crashes, even though it compiles.

This is a failure of logic, and I've got a big headache trying to figure it out. Any help would be appreciated.

Thanks in advance. :)

fgets into a temp buffer
then allocate the space
then copy the line from the temp buffer to the space allocated.

Instead of assigning counter to 0 in the following loop, I would use counter as an upper bound. That means that you would have to use another loop variable, but that shouldn't be a problem.

In your original code this is something like

fclose(openfile);

/* Print the strings in the array of pointers */
for (i = 0; i < counter; i++)

You were probably just crashing because chances are that counter is a lot smaller than NUMBEROFFILES, and at some point dereference a dangling pointer. That invokes undefined behavior, which explains your symptoms.

Thanks for everybody's help. :)

This was what was giving me a splitting headache:

Code:

---


while( fgets(files[counter], BUFSIZ, openfile) != NULL)
    {
        files[counter] = (char *)malloc( BUFSIZ * sizeof(char));  /* Try to allocate memory for string */

        if( files[counter] == NULL )
        {
            perror("\nMemory allocation error");
            exit( EXIT_FAILURE );
        }

        counter++;
    }

---

Logically, the above code is wrong.

So I took a little nap, and woke up with the solution:

Code:

---


while( (files[counter] = (char *)malloc( FILENAMESIZE * sizeof(char))) && (fgets(files[counter], BUFSIZ, openfile) != NULL))
    {
        counter++;
        lastfilenumber = counter;
    }

    if( files[counter] == NULL )
    {
        perror("\nMemory allocation error");
        exit( EXIT_FAILURE );
    }

---

Here's the entire program, and it works great. :)

Code:

---

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

#define NUMBEROFFILES 1500
#define FILENAMESIZE 500

int main( void )
{
    /* Array of pointers to store filenames */

    char *files[NUMBEROFFILES];
    int counter, lastfilenumber;
    FILE *openfile;
    char *string;

    /* Get a list of files in the directory "testwebsite", and redirect the
    output to a file called "directorylisting.txt". */

    system("dir c:\\testwebsite\\*.html > directorylisting.txt");

    /* Test to see that the file "directorylisting.txt" can be opened for reading. */

    if( (openfile = fopen("directorylisting.txt", "rt")) == NULL)
    {
        perror("\ndirectorylisting.txt");
        exit( EXIT_FAILURE );
    }

    counter = 0;

    while( (files[counter] = (char *)malloc( FILENAMESIZE * sizeof(char))) && (fgets(files[counter], BUFSIZ, openfile) != NULL))
    {
        counter++;
        lastfilenumber = counter;
    }

    if( files[counter] == NULL )
    {
        perror("\nMemory allocation error");
        exit( EXIT_FAILURE );
    }

    fclose(openfile);

    /* Print the strings in the array of pointers */

    for( counter = 0; counter < lastfilenumber; counter++)
        printf("Filename %d: %s", counter, files[counter]);

    return 0;
}

---

Code:

---

while( (files[counter] = (char *)malloc( FILENAMESIZE * sizeof(char))) &&
      (fgets(files[counter], BUFSIZ, openfile) != NULL))

---

1. There's no need to cast the result of malloc in a C program - see the FAQ.
2. Use the same size for allocating and reading. If BUFSIZ > FILENAMESIZE, then your program is on very thin ice.
3. At the end of the file, you allocate a dummy line which never gets used.
4. You don't free any of the memory.
5. The while loop should also limit counter < NUMBEROFFILES

Quote:

---

Originally Posted by Salem

Code:

---

while( (files[counter] = (char *)malloc( FILENAMESIZE * sizeof(char))) &&
      (fgets(files[counter], BUFSIZ, openfile) != NULL))

---

1. There's no need to cast the result of malloc in a C program - see the FAQ.
2. Use the same size for allocating and reading. If BUFSIZ > FILENAMESIZE, then your program is on very thin ice.
3. At the end of the file, you allocate a dummy line which never gets used.
4. You don't free any of the memory.
5. The while loop should also limit counter < NUMBEROFFILES

---

Thanks for that. :).

For #4, if I have an array of pointers ie. char *files[NUMBEROFFILES]; can I just free it with free(files)? I know how to free a memory allocated to pointer to type char, but I've never had to do it for an array of pointers to char. Does free() work exactly the same way?

I've been giving some thought to the while() loop, and I've got some questions.

Code:

---

while( (files[counter] = (char *)malloc( FILENAMESIZE * sizeof(char))) && (fgets(files[counter], BUFSIZ, openfile) != NULL))
    {
        counter++;
        lastfilenumber = counter;
    }

---

1. The first part:

Code:

---

(files[counter] = (char *)malloc( FILENAMESIZE * sizeof(char)))

---

is not really a condition? It's an assignment? Should there always be a condition(not assignment) in a while loop(or any other loop)?So while the above syntax works, is it technically incorrect?

2. I want to test that malloc successfully allocated the memory in the while() loop, but why does this:

Code:

---

while( (files[counter] = malloc( FILENAMESIZE * sizeof(char)) != NULL) && (fgets(files[counter], BUFSIZ, openfile) != NULL))

---

give me a compiler error message:

31 c:\examples\programs\test.c
warning: assignment makes pointer from integer without a cast

Also, in it's current form, is:

Code:

---

if( files[counter] == NULL )
    {
        perror("\nMemory allocation error");
        exit( EXIT_FAILURE );
    }

---

a reliable method of testing if malloc was successful, given that the test was not done in the while() loop?

Anyway, here's the cleaned up code, can you see a problem with anything else? Thanks. :)

Code:

---

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

#define NUMBEROFFILES 1500
#define FILENAMESIZE 500

int main( void )
{
    /* Array of pointers to store filenames */

    char *files[NUMBEROFFILES];
    int counter, lastfilenumber;
    FILE *openfile;
    char *string;

    /* Get a list of files in the directory "testwebsite", and redirect the
    output to a file called "directorylisting.txt". */

    system("dir c:\\testwebsite\\*.html > directorylisting.txt");

    /* Test to see that the file "directorylisting.txt" can be opened for reading. */

    if( (openfile = fopen("directorylisting.txt", "rt")) == NULL)
    {
        perror("\ndirectorylisting.txt");
        exit( EXIT_FAILURE );
    }

    counter = 1;

    while( (files[counter] = malloc( FILENAMESIZE * sizeof(char))) && (fgets(files[counter], FILENAMESIZE, openfile) != NULL)
          && (counter < NUMBEROFFILES))
    {
        counter++;
        lastfilenumber = counter;
    }

    if( files[counter] == NULL )
    {
        perror("\nMemory allocation error");
        exit( EXIT_FAILURE );
    }

    fclose(openfile);

    /* Print the strings in the array of pointers */

    for( counter = 1; counter < lastfilenumber; counter++)
        printf("Filename %d: %s", counter, files[counter]);

    free(files);

    return 0;
}

---

Quote:

---

Originally Posted by happyclown

For #4, if I have an array of pointers ie. char *files[NUMBEROFFILES]; can I just free it with free(files)? I know how to free a memory allocated to pointer to type char, but I've never had to do it for an array of pointers to char. Does free() work exactly the same way?

---

No. Every call to malloc must have its equivalent call to free. So if you call malloc and assign them to several pointers in an array, so you must call free on each of the pointers.

Quote:

---

is not really a condition? It's an assignment? Should there always be a condition(not assignment) in a while loop(or any other loop)?So while the above syntax works, is it technically incorrect?

---

Loops want a condition, true, but this is a condition.
First the assignment is performed. Then it will be dumbed down to the variable itself, and if it is non-NULL (0 basically, pretty much), is it evaluated as true; otherwise false. So it works.

Quote:

---

2. I want to test that malloc successfully allocated the memory in the while() loop, but why does this:

Code:

---

while( (files[counter] = malloc( FILENAMESIZE * sizeof(char)) != NULL) && (fgets(files[counter], BUFSIZ, openfile) != NULL))

---

give me a compiler error message:

31 c:\examples\programs\test.c
warning: assignment makes pointer from integer without a cast

---

Because it is the same as:

Code:

---

while( (files[counter] = (malloc( FILENAMESIZE * sizeof(char)) != NULL)) && (fgets(files[counter], BUFSIZ, openfile) != NULL))

---

In other words, it tests the returned pointer from malloc to see if it is true or false and assigns that boolean (which is an int) to your pointer, thus the error/warning.
The "!= NULL" part is really unnecessary, since pointers are often tested just like:

Code:

---

T* p = NULL;
if (p) /* Do something */

---

This will be false, of course.

Quote:

---

Also, in it's current form, is:

Code:

---

if( files[counter] == NULL )
    {
        perror("\nMemory allocation error");
        exit( EXIT_FAILURE );
    }

---

a reliable method of testing if malloc was successful, given that the test was not done in the while() loop?

---

It is not as reliable because you call exit, but the other code will just terminate the loop if malloc fails. This allows you to clean up, post errors messages, inform the user, try again, and quit.

Quote:

---

Originally Posted by happyclown

I have an array of pointers ie. char *files[NUMBEROFFILES]; can I just free it with free(files)?

---

Nope, you have to loop thru and free them all.

Quote:

---

Originally Posted by happyclown

I've been giving some thought to the while() loop, and I've got some questions.
Should there always be a condition(not assignment) in a while loop(or any other loop)?So while the above syntax works, is it technically incorrect?

---

There's nothing incorrect about it unless you believe that such a condition will ever be anything but true. For example, you can set up an infinite loop with while (1) {....}, because 1 is always true.

Quote:

---

Originally Posted by happyclown

2. I want to test that malloc successfully allocated the memory in the while() loop,

---

A handy and probably common way to deal with error checking malloc is to actually create a kind of proxy function that you call instead (which includes the error_check):

Code:

---

void *ec_malloc (unsigned int bytes) {
        void *assign;
        assign = malloc(bytes);       
        if (assign == NULL) {
              puts("Out of memory!!");
              exit (-666);
        }
        return assign;
}
// Example Usage:
char *ptr=ec_malloc(256);

---

Of course, you don't really have to kill yourself when malloc fails, but as they say, you might as well. I'm will admit I've actually never knowingly witnessed this happen, but then I've never tried to do it either. Anyway, at least this way you can say you caught it.

Quote:

---

Originally Posted by happyclown

31 c:\examples\programs\test.c
warning: assignment makes pointer from integer without a cast

---

Presuming files is still the array it was at the beginning of this thread, I can't say by looking why you get this warning.
Actually Elysia caught it. What she means is you have to bracket the assignment within the test:

Code:

---

while ((files[counter] = malloc( FILENAMESIZE * sizeof(char))) != NULL)

---

Quote:

---

Originally Posted by MK27

Of course, you don't really have to kill yourself when malloc fails, but as they say, you might as well. I'm will admit I've actually never knowingly witnessed this happen, but then I've never tried to do it either. Anyway, at least this way you can say you caught it.

---

Killing is a really bad idea.
If it fails, then you must clean up, sometimes try again or abort the current operation, and tell the user it failed, and preferably why.
That is why exit should never be used in utility functions.

I've learnt heaps from this thread.

Thanks for the help, everybody. :)

Quote:

---

Originally Posted by Elysia

Killing is a really bad idea.
If it fails, then you must clean up, sometimes try again or abort the current operation, and tell the user it failed, and preferably why.
That is why exit should never be used in utility functions.

---

Depends whether the code is "library" code or "program" code. A library should never call exit() (hey, libjpeg, I'm looking at you). An application can do so at its leisure.

If you are out of memory, there's really not much you can do. Trying to clean up might not even be successful, since "cleaning up" might, in itself, require allocation of memory. Hell, just popping a dialog which says "Out of memory" requires memory that you probably don't have.

One method is to allocate a "panic block" at program startup, say, a couple hundred kilobytes. If you run out of memory, you can free this block, giving yourself a little bit of breathing room. Or, if your code does some kind of advantageous caching or otherwise uses memory that isn't truly required, these layers of code can be notified of the low-memory state and they can try to dump unneeded resources.

But on any modern OS, where the kernel will clean up all program resources when the program terminates, you can just call exit() if things head south.

EDIT: You might be running out of memory, not because your program has allocated too much, but because some OTHER program has. In this case, there's absolutely NOTHING you can do, since the memory hog is probably going to continue sucking up memory. The best thing you can do when you run out of memory is to put all external data into a consistent state if you can, then terminate.

Thanks brewbuck. I agree.