Why no memory leak?

Question

The following is designed to take a variable length constant char and print it out in a nice format for logging. I am certain readers will have suggestions on how this can be improved, and I'd welcome it.

What puzzles me is that I expected it would be necessary to free() the returned static char each time ToHexString() is called. Instead, I see no memory leak whatsoever. Even tho I use the function inline and therefore do not assign its return value to a variable.

I created a simple test that calls this function in a loop, each time with a different length cString and nMaxChars parameter. Then I watched VM status. The memory allocation for my test program, and free memory, never changed.

It seems to me it should have increased each time a malloc is called and no free.

static char *ToHexString(const char *cString,int nMaxChars)
{
    static char *cStr;



    /*if (80>strlen(cString))
        nRawChars=strlen(cString);
    if (nMaxChars>nRawChars)
        nRawChars=nMaxChars;
    */
    if (nMaxChars==0)
        nMaxChars=80;

    printf("There are %i chars\n",nMaxChars);

    char *cStr1;
    char *cStr2;
    char *cStr3;
    int nLen=nMaxChars*6;
    cStr=calloc(nLen,sizeof(char));

    cStr1=calloc(10,sizeof(char));
    cStr2=calloc(nLen,sizeof(char));
    cStr3=calloc(nLen,sizeof(char));
    cStr1[0]='\0';
    cStr2[0]='\0';
    cStr3[0]='\0';
    int nC1=0;
    int nRowCnt=0;

    for (nC1=0;nC1<nMaxChars;nC1++)
    {
        ++nRowCnt;
        if (cString[nC1]==0x00)
            snprintf(cStr1,8,"[00] ");
        else
            snprintf(cStr1,8,"[%02x] ",(unsigned char)cString[nC1]);

        if ( (nRowCnt%8==0) )
        {
            snprintf(cStr3,nLen,"%s%s\n",cStr2,cStr1);
        }
        else
            snprintf(cStr3,nLen,"%s%s",cStr2,cStr1);

        snprintf(cStr2,nLen,"%s",cStr3);
    }
    snprintf(cStr,nLen,"%s",cStr3);
    free(cStr1);
    free(cStr2);
    free(cStr3);
    return(cStr);
}

Here is the calling routine:

for (i=0;i<100;i++)
{
    memset(&cBuff, 0,255);
    printf("Reading %s now..\n",cPort);
    while (sleep(1)==-1);
    nChars=read(nPort, cBuff, 255);
    //printf("Read %i chars from %s\n",nChars,cPort);
    if (nChars<=0)
        printf("Read 0 chars from %s\n",cPort);
    else
        printf("Read %i chars from %s\n%s\n",nChars,cPort,ToHexString(cBuff,nChars));
}

Answer 1

The following is a leak:

static void memeat(void)
{
        static char *foo = NULL;

        foo = malloc(1024);

        return;

}

Valgrind output:

==16167== LEAK SUMMARY:
==16167==    definitely lost: 4,096 bytes in 4 blocks
==16167==    indirectly lost: 0 bytes in 0 blocks
==16167==      possibly lost: 0 bytes in 0 blocks
==16167==    still reachable: 1,024 bytes in 1 blocks
==16167==         suppressed: 0 bytes in 0 blocks
==16167== Rerun with --leak-check=full to see details of leaked memory

Note, still reachable (1024 bytes) is the result of the last time memeat() was entered. The static pointer still had a valid reference to the last block memeat() allocated when the program exited. Just not the previous blocks.

The following is NOT a leak:

static void memeat(void)
{
        static char *foo = NULL;

        foo = realloc(foo, 1024);

        return;

}

Valgrind output:

==16244== LEAK SUMMARY:
==16244==    definitely lost: 0 bytes in 0 blocks
==16244==    indirectly lost: 0 bytes in 0 blocks
==16244==      possibly lost: 0 bytes in 0 blocks
==16244==    still reachable: 1,024 bytes in 1 blocks
==16244==         suppressed: 0 bytes in 0 blocks
==16244== Rerun with --leak-check=full to see details of leaked memory

Here, the address foo pointed to has been freed, and foo now points to the newly allocated address, and will continue to do so the next time memeat() is entered.

Explanation:

The static storage type says that the pointer foo will point to the same address as initialized each time the function is entered. However, if you change that address each time the function is entered via malloc() or calloc(), you've lost the reference to the blocks from the previous allocation. Hence, a leak, since either is going to return a new address.

'Still Reachable' in valgrind means that that all allocated heap blocks still have a valid pointer to access / manipulate / free them upon exit. This is similar to allocating memory in main() and not freeing it, just relying on the OS to reclaim memory.

In short, yes - you have a leak, however you can fix it rather easily. Just note, you are indeed relying on your OS to reclaim it, unless you add another argument to your function that just tells ToHexString to call free on the static pointer, which you could use when exiting.

Similar to this: (full test program)

#include <stdlib.h>

static void memeat(int dofree)
{
        static char *foo = NULL;

        if (dofree == 1 && foo != NULL) {
                free(foo);
                return;
        }

        foo = realloc(foo, 1024);

        return;

}


int main(void)
{
        unsigned int i;

        for (i = 0; i < 5; i ++)
                memeat(0);

        memeat(1);
        return 0;
}

Valgrind output:

==16285== HEAP SUMMARY:
==16285==     in use at exit: 0 bytes in 0 blocks
==16285==   total heap usage: 6 allocs, 6 frees, 6,144 bytes allocated
==16285==
==16285== All heap blocks were freed -- no leaks are possible

Note on the final output:

Yes, 6144 bytes were actually allocated according to what malloc() returned while the program ran, but that just means the static pointer was freed, then reallocated according to the number of times memeat() was entered. The actual heap use of the program at any given time was actually just 2*1024, 1k to allocate the new pointer while the old one still existed waiting to be copied to the new one.

Again, it should not be too hard to adjust your code, though I'm not sure why you are using static storage to begin with. I think you may be falling victim to premature / micro optimization based on your comments.

Answer 2

You return the result of your routine in a fresh array. With your model, the responsibility to free this array with the result is in the caller. So there, in the caller, you should store the result of your routine in a temporary, do whatever you want with it, and then free() it at the end.

Answer 3

It is a memory leak. If you persistently call the function the memory used by the program increases. For example:

int main() {
   while (1) {
      ToHexString("testing the function", 20);
   }
}

If you run this and watch the process with a system monitoring tool you will see that the used memory is constantly increasing.

The leak is probably not obvious in your program because the function only leaks a few bytes each call and is not called very often. So the increase in memory usage is not very noticeable.

Answer 4

I'd like to point out two things that crosses my mind when inspecting the code:

cStr=calloc(nLen,sizeof(char));

Why did you not do error checking on this.... as I can see from the code, there is zero checking on the assumption memory WILL always be available....dangerous.... ALWAYS check for NULL pointer on return from a memory allocation function call such as calloc, malloc and realloc...it will ALWAYS be the onus on the programmer to manage the free'ing up of pointers to return them back to the heap.

Also, because you have cStr declared as a static pointer to char *, you are not freeing it up at all in fact this line proves it:

printf("Read %i chars from %s\n%s\n",nChars,cPort,ToHexString(cBuff,nChars));
                                                  ^^^^^^^^^^^

You would be better off to do it this way:

char *hexedString;
hexedString = ToHexString(cBuff, nChars);
....
printf("Read %i chars from %s\n%s\n",nChars,cPort,hexedString);
....
free(hexedString);