I've just created a memory leak

zaichik · 09-04-2005, 04:45 PM

Hi all,

It just occurred to me that this function creates a memory leak, does it not?

Code:

char* int_to_dotted( unsigned int ipv4 ) {

        int remainder = 0;
        int octet1 = 0, octet2 = 0, octet3 = 0;
        unsigned int FIRST_OCT_MULTIPLIER = 16777216;
        unsigned int SECOND_OCT_MULTIPLIER = 65536;
        unsigned int THIRD_OCT_MULTIPLIER = 256;
        char *answer;

        answer = ( char * ) malloc( MAX_SIZE_IPADDR + 1 );
        if( answer == NULL ) {
                printf( "%s:  Out of memory in int_to_dotted().\n", myName );
                exit( -1 );
        }

        octet1 = ipv4 / FIRST_OCT_MULTIPLIER;
        ipv4 %= FIRST_OCT_MULTIPLIER;
printf( "ipv4 = %d\n", ipv4 );
        octet2 = ipv4 / SECOND_OCT_MULTIPLIER;
        ipv4 %= SECOND_OCT_MULTIPLIER;
printf( "ipv4 = %d\n", ipv4 );
        octet3 = ipv4 / THIRD_OCT_MULTIPLIER;
        ipv4 %= THIRD_OCT_MULTIPLIER;
printf( "octet1 = %d, o2 = %d, o3 = %d, o4 = %d\n", octet1, octet2, octet3, ipv4 );
        sprintf( answer, "%d.%d.%d.%d", octet1, octet2, octet3, ipv4 );
printf( "answer = %s\n", answer );
        return( answer );
}

By having it return the a variable that is dynamically allocate, I cannot free this memory. So every time it gets called, I am leaking away MAX_SIZE_IPADD + 1.

Can anyone think of a way around this, short of passing a char* to the function to store the value in?

TIA!

spooon · 09-04-2005, 04:51 PM

You can always just free() the value returned by this function whenever you're done with it; since the return value ("answer") is just the pointer to the memory you allocated.

zaichik · 09-04-2005, 05:15 PM

Hi, thanks for the reply.

OK, what I have now is this:

Code:

int callingFunction( void ) {
   char *ip;
   ip = ( char * ) malloc( MAX_SIZE_IPADDR );
   sprintf( ip, "%s", int_to_dotted( someInt );
}

So you are saying to do this:

Code:

int callingFunction( void ) {
   char *ip;
   ip = int_to_dotted( someInt );
   .
   .
   free( ip );

And that 1) work, and 2) free the memory allocated in the other function?

Thanks for your reply, again!

btmiller · 09-04-2005, 10:25 PM

There are a couple of cardinal rules to malloc/free

(1) One free per malloc.
(2) Only ever pass free a pointer you got with malloc. Passing it other stuff will lead to weird results (and program crashes).

So, yes, you can malloc some memory in one function and free it in another function. But don't do something like:

Code:

char *foo;

foo = malloc(SOMESIZE);
foo = malloc(SOMETHINGELSE);
free(foo);

Dpo you see why? That code called malloc twice (on the same pointer, but freed only the second chunk. The first chunk was effectively lost, and this is bad.

zaichik · 09-04-2005, 10:32 PM

Hi,

Yes, I see why that is bad. But this:

Code:

char *a;
char *b;

b = malloc( someValue );
a =b;
free( a );

would work just fine. Since, after all, it's just a memory location, passing it to free by whatever name would have the same effect--assuming that memory address had been malloc'd in the first place.

Thanks for your help!

sind · 09-04-2005, 11:54 PM

Hi zaichik,

IMO, your best bet would be to pass int_to_dotted() a pointer to a chunk of memory, along with the size of that chunk; that way you can keep any calls to malloc() and free() in the calling function, which might help with debugging. This seems to be the convention for thread-safe functions in glibc.

If you're going to do this:

Code:

char *ip;
ip = ( char * ) malloc( MAX_SIZE_IPADDR );
sprintf( ip, "%s", int_to_dotted( someInt );

you might as well pass the malloc()'d pointer to int_to_dotted() and save an sprintf() (I'd recommend that you use snprintf(), BTW), as well saving an additional block of memory, of size (MAX_SIZE_IPADDR + 1). Just a thought.

Also, I'm interested to know why you are casting the result of malloc(), it shouldn't be necessary.

Code:

void int_to_dotted( unsigned int ipv4, char *answer, unsigned int answer_len ) {

    int remainder = 0;
    int octet1 = 0, octet2 = 0, octet3 = 0;
    unsigned int FIRST_OCT_MULTIPLIER = 16777216;
    unsigned int SECOND_OCT_MULTIPLIER = 65536;
    unsigned int THIRD_OCT_MULTIPLIER = 256;

    octet1 = ipv4 / FIRST_OCT_MULTIPLIER;
    ipv4 %= FIRST_OCT_MULTIPLIER;
    printf( "ipv4 = %d\n", ipv4 );

    octet2 = ipv4 / SECOND_OCT_MULTIPLIER;
    ipv4 %= SECOND_OCT_MULTIPLIER;
    printf( "ipv4 = %d\n", ipv4 );

    octet3 = ipv4 / THIRD_OCT_MULTIPLIER;
    ipv4 %= THIRD_OCT_MULTIPLIER;
    printf( "octet1 = %d, o2 = %d, o3 = %d, o4 = %d\n", octet1, octet2, octet3, ipv4 );

    snprintf( answer, answer_len, "%d.%d.%d.%d", octet1, octet2, octet3, ipv4 );

    printf( "answer = %s\n", answer );
}


int callingFunction( void ) {

    char *ip;
    ip = malloc( MAX_SIZE_IPADDR );
    
    /* malloc error check */

    int_to_dotted( someInt, ip, MAX_SIZE_IPADDR );

    /* Do something with ip */

    if ( ip != NULL ) {
        free( ip );
        ip = NULL;
    }

    /* Rest of fn */
}

~sind

zaichik · 09-05-2005, 06:36 AM

Quote:

you might as well pass the malloc()'d pointer to int_to_dotted() and save an sprintf() (I'd recommend that you use snprintf(), BTW), as well saving an additional block of memory, of size (MAX_SIZE_IPADDR + 1).

Well, that makes sense, and honestly, I don't know why I didn't just do that from the beginning.

What is the point of setting the pointer equal to NULL after it is freed? Is that to prevent accidentally trying to access the formerly pointed-to memory again?

And casting the return value of malloc()? I've always done that, as that was highly recommended in the book in which I initially learned how to use malloc, "Linux Programming by Example" by Arnold Robbins:

Quote:

Allocate the storage by calling malloc(), assigning the function's return value to the pointer variable. It is good practice to cast the return value of malloc() to that of the variable being assigned to. In C it's not required (although the compiler may generate a warning). We strongly recommend always casting the return value.

Thoughts?