newbie: need help using malloc in a loop

This is a discussion on newbie: need help using malloc in a loop within the C Programming forums, part of the General Programming Boards category; Hi all. Here are my objectives: 1. Redirect a list of files in a directory into a text file. 2. ...

  1. #1
    In my head happyclown's Avatar
    Join Date
    Dec 2008
    Location
    In my head
    Posts
    391

    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.

  2. #2
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,558
    fgets into a temp buffer
    then allocate the space
    then copy the line from the temp buffer to the space allocated.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  3. #3
    Registered User whiteflags's Avatar
    Join Date
    Apr 2006
    Location
    United States
    Posts
    7,709
    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.

  4. #4
    In my head happyclown's Avatar
    Join Date
    Dec 2008
    Location
    In my head
    Posts
    391
    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;
    }

  5. #5
    and the hat of wrongness Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    32,558
    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
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.
    I support http://www.ukip.org/ as the first necessary step to a free Europe.

  6. #6
    In my head happyclown's Avatar
    Join Date
    Dec 2008
    Location
    In my head
    Posts
    391
    Quote Originally Posted by Salem View Post
    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;
    }
    Last edited by happyclown; 01-03-2009 at 06:59 AM. Reason: typo

  7. #7
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,629
    Quote Originally Posted by happyclown View Post
    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.

    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.

    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.

    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 Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  8. #8
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Quote Originally Posted by happyclown View Post
    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 View Post
    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 View Post
    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 View Post
    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)
    Last edited by MK27; 01-03-2009 at 07:30 AM.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

  9. #9
    C++まいる!Cをこわせ! Elysia's Avatar
    Join Date
    Oct 2007
    Posts
    22,629
    Quote Originally Posted by MK27 View Post
    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.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  10. #10
    In my head happyclown's Avatar
    Join Date
    Dec 2008
    Location
    In my head
    Posts
    391
    I've learnt heaps from this thread.

    Thanks for the help, everybody.

  11. #11
    Captain Crash brewbuck's Avatar
    Join Date
    Mar 2007
    Location
    Portland, OR
    Posts
    7,243
    Quote Originally Posted by Elysia View Post
    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.
    Last edited by brewbuck; 01-03-2009 at 04:37 PM.
    Code:
    //try
    //{
    	if (a) do { f( b); } while(1);
    	else   do { f(!b); } while(1);
    //}

  12. #12
    spurious conceit MK27's Avatar
    Join Date
    Jul 2008
    Location
    segmentation fault
    Posts
    8,300
    Thanks brewbuck. I agree.
    C programming resources:
    GNU C Function and Macro Index -- glibc reference manual
    The C Book -- nice online learner guide
    Current ISO draft standard
    CCAN -- new CPAN like open source library repository
    3 (different) GNU debugger tutorials: #1 -- #2 -- #3
    cpwiki -- our wiki on sourceforge

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 8
    Last Post: 12-01-2008, 09:09 AM
  2. Mutiliplcation Program that uses a (do, while, and for loop)?
    By Debbie Bremer in forum C++ Programming
    Replies: 4
    Last Post: 10-11-2008, 06:04 PM
  3. Can a "switch" be inside a loop?
    By gmk0351 in forum C Programming
    Replies: 5
    Last Post: 03-28-2008, 05:47 PM
  4. I need help as soon as possible.
    By hyrule in forum C++ Programming
    Replies: 7
    Last Post: 11-09-2005, 04:49 PM
  5. How to change recursive loop to non recursive loop
    By ooosawaddee3 in forum C Programming
    Replies: 1
    Last Post: 06-24-2002, 08:15 AM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21