Thread: Error using realloc inside function

  1. #1
    Registered User
    Join Date
    Mar 2012
    Posts
    16

    Error using realloc inside function

    Happy holidays C gurus! I am running into a strange error using realloc to resize a 2D array within a function and could use some advice.

    The function is designed to remove duplicate entries from a dynamically allocated 2D array of integers with 3 columns. The function is used inside a for loop and gives the illusion of working for the first 4 repeats. On the fifth repeat, malloc returns a NULL value at the point indicated in the code below. The error occurs at exactly the same point every time I run the simulation on a LINUX machine. However, I get no error messages running the same code on a Windows machine.

    I know it is not a particularly elegant function, but are there any obvious memory leaks or is it something to do with the machine itself?

    Thanks in advance for your help.

    Code:
    
    
    Code:
    int** condense_rewired(int **rewired_edges, long int num_rewired_edges)
    {
     
       /* QSORT LIST BY COL 3, COL 1, COL 2 */
     int comp_rewired(const void* a, const void* b) 
           {
             int **p1 = (int**)a;
             int **p2 = (int**)b;
             int *arr1 = *p1;
             int *arr2 = *p2;
      
             int diff3 = arr1[2] - arr2[2];
               if (diff3) return diff3;
             int diff2 = arr1[0] - arr2[0];
              if (diff2) return diff2;
             return arr1[1] - arr2[1];
           }
     
     qsort(rewired_edges, num_rewired_edges, sizeof(rewired_edges[0]), comp_rewired);
    
    
        int **temp_edges = (int**)malloc( sizeof(int *) * 1);
             temp_edges[0] =(int*)malloc( sizeof(int) * 3); 
     
    long int x = 0;
    long int num_temp_edges = 1;
      
     temp_edges[0][0] = rewired_edges[0][0];
     temp_edges[0][1] = rewired_edges[0][1];
     temp_edges[0][2] = rewired_edges[0][2];
         
     for(x = 1; x < num_rewired_edges; x++)
        {   
                if( rewired_edges[x][0] == rewired_edges[(x - 1)][0] && rewired_edges[x][1] == rewired_edges[(x - 1)][1] 
                           && rewired_edges[x][2] == rewired_edges[(x - 1)][2] ) 
                   {
                    
                   }
                else
                  {
                     temp_edges = (int **)realloc(temp_edges, sizeof(int*) * (num_temp_edges + 1));
                     temp_edges[num_temp_edges] = (int*)malloc(sizeof(int) * 3);
                     if(temp_edges[num_temp_edges] == NULL)
                            {
                               printf(" temp_edges malloc fail \n");
                             }                  
                     temp_edges[num_temp_edges][0] = rewired_edges[x][0];
                     temp_edges[num_temp_edges][1] = rewired_edges[x][1];
                     temp_edges[num_temp_edges][2] = rewired_edges[x][2];
                    
                     num_temp_edges = num_temp_edges + 1;             
                    
                  }                           
          }    
          
    
       rewired_edges = (int **)realloc(rewired_edges, sizeof(int*) * (num_temp_edges));
       for(x = 0; x < num_temp_edges; x++)
         {
           rewired_edges[x] = (int*)malloc(sizeof(int) * 3);
           if( rewired_edges[x] == NULL)
             {
                   printf("malloc failed here, x = %ld \n", x); /* **** CRASHES HERE  ****  */
             }
           rewired_edges[x][0] = temp_edges[x][0];
           rewired_edges[x][1] = temp_edges[x][1];
           rewired_edges[x][2] = temp_edges[x][2];
         }
     
         num_rewired_edges = num_temp_edges;
         
         for(x = 0; x < num_temp_edges; x++)
             { free(temp_edges[x]); } 
             free(temp_edges);  
         
    
     return(rewired_edges);
    
        
    }

  2. #2
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    It would help if you posted a small, complete, and COMPILABLE sample of code that (when compiled, linked, and run) illustrates your problem, rather than copying and pasting snippets at random that you think are relevant.

    As is, your code has a function implemented inside a function, which is illegal in C. You have not also provided any code which calls your functions, so nobody has a hope of recreating your symptoms. A main() function is important as you claim to have a runtime problem.

    All I can do is guess.

    My guess is that rewired_edges passed from the caller is not initialised before the first call. There is also a memory leak as, when reallocating rewired_edges, your code loses track of the elements of rewired_edges (i.e. memory leak). That does not typically cause a failure of malloc() directly - unless it causes memory exhaustion.

    If it is not that, read this link before posting again.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  3. #3
    Registered User
    Join Date
    Nov 2010
    Location
    Long Beach, CA
    Posts
    5,909
    You do have a potential memory leak, as well as some other issues, so here is a list of things I came across in a fairly quick review

    • Your indentation and code formatting is inconsistent. Clean, consistent code is easier to read and write. This makes it harder to make mistakes and easier to find and fix them when you do.
    • C does not support nested functions, you will have to move comp_rewired outside condense_rewired.
    • Don't cast the return value of malloc/realloc. Read this link.
    • You never check the return value of malloc/realloc, you just assume it works. That could result in seg faults should malloc/realloc fail and you try to access temp_edges[x] or some such.
    • You should
    • You have a memory leak in your realloc usage. You need a teporary variable to hold the return of realloc. If you do foo = realloc(foo, size), and realloc fails, then you lost the pointer to the original piece of memory. Not only did you leak memory, you lost the data you still need. See below for the correct idiom.
    • You should #define COLS 3, or some such, and use that instead of using magic numbers and manipulating array elements individually (arrays almost always should be used in conjunction with loops). For example, lines 28-30, 47-49, 65-67 and probably more, could be done in a loop. Better yet, you could create a function called something like copy_edge, that does all the work, and call that in place of those 3 individual assignments. It makes your code more flexible, should you decide to handle 4-column data, and even if you know you wont need 4 columns, it's good design and a good habit to get into.


    Here's proper realloc usage (with improved sizeof parameter):
    Code:
    int *foo;
    int *temp;
    
    // allocate N elements
    foo = malloc(N * sizeof(*foo));  // if foo ever changes it's type, the malloc/realloc calls can stay the same
    if (!foo)
        // print an error and exit
    ...
    // not enough space, realloc M elements
    temp = realloc(foo, M * sizeof(*foo));
    if (temp)
        foo = temp;
    else
        realloc failed, do something appropriate...at least foo still points to the old memory, nothing is lost
    There's probably more, but that should keep you going for a bit.

  4. #4
    Registered User
    Join Date
    Mar 2012
    Posts
    16
    This is a much simplified version of the way the function is currently called in the program, but compilable.

    I am a little bit confused by:
    "There is also a memory leak as, when reallocating rewired_edges, your code loses track of the elements of rewired_edges (i.e. memory leak)."

    I thought realloc automatically released the memory when it resizes the array.

    Code:
     /* ---------------------------------------------------------------------------*/
    /* LIBRARIES ---------------------------------------------------------------- */
    #include <stdio.h>
    #include <math.h>
    #include <stdlib.h>
    #include <time.h>
    #include <string.h>
    
    
    /* --------------------------------------------------------------------------- */
    /* GLOBAL DEFINITIONS ------------------------------------------------------ */
    
    int** condense_rewired();
    long int num_rewired_edges;
    
    
    /* --------------------------------------------------------------------------- */
    /*MAIN PROGRAM ------------------------------------------------------ */
    
    int main(void)
    {
       
        srand(time(NULL));   
        long int i = 0;
        long int x = 0;
    
           for(x = 0; x < 25; x++)
              {
                 num_rewired_edges = 150000;
                 
                 int **rewired_edges = (int**)malloc( sizeof(int *) * num_rewired_edges);
                 for(i = 0; i < num_rewired_edges; i++)
                   {
                     rewired_edges[i] =(int*)malloc( sizeof(int) * 3); 
                     rewired_edges[i][0] = rand() % 100; 
                     rewired_edges[i][1] = rand() % 100; 
                     rewired_edges[i][2] = rand() % 50; 
                  }
              
                rewired_edges = condense_rewired(rewired_edges);
                printf( "rep: %ld, num_rewired_edges: %ld\n", i, num_rewired_edges);         
    
                for(i = 0; i < num_rewired_edges; i++)
                  { free(rewired_edges[i]); } 
                  free(rewired_edges);  
              
              }
             
    
    return (0);
    }
    
    
    /* --------------------------------------------------------------------------- */
    /*FUNCTION DEFINITIONS ------------------------------------------------------ */
    
    int** condense_rewired(int **rewired_edges)
    {
      
       /* QSORT LIST BY COL 3, COL 1, COL 2 */
     int comp_rewired(const void* a, const void* b)
           {
             int **p1 = (int**)a;
             int **p2 = (int**)b;
             int *arr1 = *p1;
             int *arr2 = *p2;
       
             int diff3 = arr1[2] - arr2[2];
               if (diff3) return diff3;
             int diff2 = arr1[0] - arr2[0];
              if (diff2) return diff2;
             return arr1[1] - arr2[1];
           }
      
     qsort(rewired_edges, num_rewired_edges, sizeof(rewired_edges[0]), comp_rewired);
     
     
        int **temp_edges = (int**)malloc( sizeof(int *) * 1);
             temp_edges[0] =(int*)malloc( sizeof(int) * 3);
      
    long int x = 0;
    long int num_temp_edges = 1;
       
     temp_edges[0][0] = rewired_edges[0][0];
     temp_edges[0][1] = rewired_edges[0][1];
     temp_edges[0][2] = rewired_edges[0][2];
          
     for(x = 1; x < num_rewired_edges; x++)
        {  
                if( rewired_edges[x][0] == rewired_edges[(x - 1)][0] && rewired_edges[x][1] == rewired_edges[(x - 1)][1]
                           && rewired_edges[x][2] == rewired_edges[(x - 1)][2] )
                   {
                     
                   }
                else
                  {
                     temp_edges = (int **)realloc(temp_edges, sizeof(int*) * (num_temp_edges + 1));
                     temp_edges[num_temp_edges] = (int*)malloc(sizeof(int) * 3);
                     if(temp_edges[num_temp_edges] == NULL)
                            {
                               printf(" temp_edges malloc fail \n");
                             }                 
                     temp_edges[num_temp_edges][0] = rewired_edges[x][0];
                     temp_edges[num_temp_edges][1] = rewired_edges[x][1];
                     temp_edges[num_temp_edges][2] = rewired_edges[x][2];
                     
                     num_temp_edges = num_temp_edges + 1;            
                     
                  }                          
          }   
           
     
       rewired_edges = (int **)realloc(rewired_edges, sizeof(int*) * (num_temp_edges));
       for(x = 0; x < num_temp_edges; x++)
         {
           rewired_edges[x] = (int*)malloc(sizeof(int) * 3);
           if( rewired_edges[x] == NULL)
             {
                   printf("malloc failed here, x = %ld \n", x); /* **** CRASHES HERE  ****  */
             }
           rewired_edges[x][0] = temp_edges[x][0];
           rewired_edges[x][1] = temp_edges[x][1];
           rewired_edges[x][2] = temp_edges[x][2];
         }
      
         num_rewired_edges = num_temp_edges;
          
         for(x = 0; x < num_temp_edges; x++)
             { free(temp_edges[x]); }
             free(temp_edges); 
          
     
     return(rewired_edges);
     
         
    }

  5. #5
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    Quote Originally Posted by NotAProgrammer View Post
    This is a much simplified version of the way the function is currently called in the program, but compilable.
    What you have posted is not compilable as C. It has a function implementation inside the body of another function. Either you are using another language that looks sort-of like C but isn't, or you are using a non-standard compiler extension that accepts code like that.

    Quote Originally Posted by NotAProgrammer View Post
    I am a little bit confused by:
    "There is also a memory leak as, when reallocating rewired_edges, your code loses track of the elements of rewired_edges (i.e. memory leak)."

    I thought realloc automatically released the memory when it resizes the array.
    It does. However, if realloc() fails, it returns NULL and the original memory is not released. The caller needs to cope when that happens. Your code is not even testing for that occurrence, let alone recovering from it.

    When you have memory leaks, which your code does (multiple malloc() calls with no corresponding realloc() or free()), such an occurrence is significantly more likely.


    You might want to check the return value from malloc() inside main(). malloc(150000*sizeof(int *)) is not guaranteed to succeed. If you are using a 16-bit compiler, it WILL fail. Your code is not checking for that occurrence. Even worse, you're doing that in a loop, with other code that has several memory leaks.

    You have also not corrected several of the concerns pointed out by anduril.
    Last edited by grumpy; 12-26-2012 at 07:04 PM.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  6. #6
    Algorithm Dissector iMalc's Avatar
    Join Date
    Dec 2005
    Location
    New Zealand
    Posts
    6,318
    My homepage
    Advice: Take only as directed - If symptoms persist, please see your debugger

    Linus Torvalds: "But it clearly is the only right way. The fact that everybody else does it some other way only means that they are wrong"

  7. #7
    Registered User
    Join Date
    Jun 2005
    Posts
    6,815
    Quote Originally Posted by iMalc View Post
    Possibly. However, when a compiler implements something that the standard does not support, it is the compiler that is wrong (or, at best, extended) not the standard.
    Right 98% of the time, and don't care about the other 3%.

    If I seem grumpy or unhelpful in reply to you, or tell you you need to demonstrate more effort before you can expect help, it is likely you deserve it. Suck it up, Buttercup, and read this, this, and this before posting again.

  8. #8
    Registered User
    Join Date
    Mar 2012
    Posts
    16
    I gave up trying to get realloc to work in the for loop without leaking memory and instead re-wrote the code using linked lists to temporarily store the new array elements. At the end of the for loop, I just malloc the 2D array once using the number of elements stored in the linked list, populate the array with the values stored in the structs, and then delete the linked list. Works like a charm and according to Valgrind, I am no longer leaking memory.

    I am using GCC through Cygwin as my compiler, which does allow nested functions.
    Last edited by NotAProgrammer; 12-28-2012 at 04:01 AM.

  9. #9
    Registered User
    Join Date
    Nov 2012
    Posts
    1,393
    Quote Originally Posted by NotAProgrammer View Post
    I am using GCC through Cygwin as my compiler, which does allow nested functions.
    In your example, your comp_rewired function does not need to be nested. Why not just define it as a normal function? Make it a static function if your goal is to limit access to the function or to avoid name conflicts. Static functions are only callable from other functions in the same compilation unit. This standard technique negates the need for GCC nested functions.

  10. #10
    Registered User
    Join Date
    Nov 2012
    Posts
    32
    Memory leak is at line 113 of your last listing, because old int[3] arrays isn't freed.
    Insert rewired_edges cleaning as at lines 43-44 before this line.
    Last edited by Shurik; 12-28-2012 at 10:15 AM.

  11. #11
    Registered User
    Join Date
    Dec 2012
    Posts
    45
    Quote Originally Posted by NotAProgrammer View Post
    I gave up trying to get realloc to work in the for loop without leaking memory and instead re-wrote the code using linked lists [..]
    It's a pity, but at least you probably learned a lot about dynamic memory and linked lists :-)

    That said, your problem has a much more simple solution. You only need one single block of dynamic memory!

    First, you should embed the array (pointer and number of elements) in a structure. Then you can pass this structure by reference to the function condense_rewired():

    Code:
    typedef struct
    {
        long num;      // Number of elements
        int (*p)[3];   // 2D array with 3 columns and 'num' rows
    }
    rewired_edges_t;
     
    void condense_rewired (rewired_edges_t *);
    Then you can declare the structure as a local variable in main(), and create the array with a single malloc():

    Code:
    int main (void)
    {
        //...
        rewired_edges_t re;
        
        re.p = (int(*)[3]) malloc (sizeof(int[3])*SIZE);
     
        for(x = 0; x < 25; x++)
        {
            re.num = SIZE;
    
            //...
    
            condense_rewired (&re);
    Then, condese_rewired() can sort the rows of the array with qsort(), and eliminate the duplicates with an algorithm like this:

    Code:
    int a, b, n, X[10]={1,2,2,2,3,4,4,6,6,7};
    
    for (n=10, a=b=1; b<n; b++)
        if (X[b]!=X[a-1])
        {
            X[a] = X[b];
            a ++;
        }
    
    n = a;  // Number of unique elements
    Last edited by comocomocomo; 12-29-2012 at 08:22 AM. Reason: I submitted the uncomplete text by mistake

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. realloc inside a routine
    By coletek in forum C Programming
    Replies: 4
    Last Post: 06-04-2009, 09:00 PM
  2. call to realloc() inside a function: memory problem
    By simone.marras in forum C Programming
    Replies: 15
    Last Post: 11-30-2008, 10:01 AM
  3. Error when freeing memory (allocated with realloc)
    By svdrjeug in forum C Programming
    Replies: 18
    Last Post: 01-03-2008, 11:16 AM
  4. Realloc problems with sturcture array inside structure
    By daveyand in forum C Programming
    Replies: 2
    Last Post: 03-29-2004, 06:48 AM
  5. Problem with realloc in the following function
    By Little_Dump in forum C Programming
    Replies: 5
    Last Post: 10-27-2003, 02:16 PM