memory leak after free the memory for char string

[can-mohan]

Hi All,
in below code snippet, I have been facing memory leak issue while free memory for string.Could you let me know thereason for the same.I believe if we are allocating memory then it needs to be freedbut what i observed if i don't free the memory it works fine
but while freeing the memory to allocated string it is getting leaked.

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#define RESULT_FAILURE -1
#define RESULT_SUCCESS 0

int16_t  fun(char *iaddr)
{  char* p;
     int rc;
  if(strlen(iaddr)>0)
  {
  p=(char*)malloc(strlen(iaddr)+1);
strcpy(p,iaddr);
  }
  else
  {
  rc=RESULT_FAILURE;
  free(p);
  }

   while(*p)
  {


p++;
printf("%c",*p);

}

free(p);//memory leak happens here
}

int main()
{

int rc;

rc= fun("10.3.28");

printf("irc=%d",rc);

return 1;
}

[skaa]

You changed p by p++;, so you cannot free new p. :)

[can-mohan]

Thanks for your reply . This string is created on heap so how memory will be released for the same as every malloc we must have corrosponding free in order to free the allocated memory.

[Moschops]

When you call free, you must use a pointer with the same value as the one that was given to you by the corresponding malloc.

Here is how to copy a pointer so that you can keep one with the same value. In this sample code, pointer is the pointer than was returned by malloc.

copy_of_pointer = pointer;

[rubberman]

Other comments. The signature of fun() is this: int16_t fun(char* iaddr); However, you never return anything from the function, which is what the int rc is intended for. Also, you should initialize rc to a sane value when you declare it. Next, the iaddr argument should be a const char* and not a char*. Finally, you can use strdup() instead of malloc()+strcpy() which will take care of the terminating null properly for you.

Moschops is 100% in that once you change the pointer p with p++ it is no longer pointing at an address that free will work with. You need to assign p to another pointer first, and then free that one. To avoid all of this, you can use alloca() + strcpy() which will allocate your memory on the stack, which will automatically cleaned up when the function terminates and the stack unwinds. One major advantage of that is that you don't need to use free().

[can-mohan]

Thanks for your explanation rubberman. I got to know that strdup is treated as evil as it never free the memory. Does it use alloca()+strcpy() concept.

[Moschops]

No. You are expected to call free yourself on the pointer returned from strdup.

[rubberman]

What Moschops said. The call to strdup() is equalevent to malloc(strlen+1)+strcpy. It does the allocation and copy, so it does 2 things: reduce code by one function, and deals with proper string length computations - it may also incorporate other optimizations. Who said it was "evil"? Whoever it was is ignorant and does not understand how it works. Just my (not so) humble opinion. And FWIW, I have been professionally programming C for over 30 years, and C++ for over 20 years, and that includes a patent in adaptive systems engineering plus invited papers at a number of major international software engineering conferences.

[can-mohan]

but how strdup will release the memory or user needs to release memory by calling free function.so in my below code if i replace the 2 lines of code (memory allocation+content copying) with strdup, then do i need to free memory explicity or strdup will take care of the same.

if(strlen(iaddr)>0)
11.  {
12.  p=strdup(iaddr)
13.
14.  }



    while(*p)
22.  {
23.
24.
25.p++;
26.printf("%c",*p);
27.
28.}


free(p)// I don't thing so still free can be used here

@moschop:
apart from this as we are incrementing the pointer then again internal memory address will be changed and we can't free the memory despite of strdup function as i don't thing so again pointer will have same values that was given by malloc . Please correct me if i am wrong

[VatooVatoo]

Maybe you could use somthing like this:

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
#define RESULT_FAILURE -1
#define RESULT_SUCCESS 0

int16_t  fun(char *iaddr)
{  
    char* p = NULL;
    int i = 0;
    int len = 0;
    int rc = RESULT_SUCCESS;

    if(strlen(iaddr)>0)
    {
        len = strlen(iaddr)+1;
        p = (char*)malloc(len );
        strcpy(p,iaddr);

        for(i=0;i<len;i++)
        {
            printf("%c", p[i]);
        }

        free(p);
    }
    else
    {
        rc = RESULT_FAILURE;
    }

    return rc;
}

int main()
{
    int rc;
    rc= fun("10.3.28");
    printf("irc=%d",rc);
    return 1;
}

[Moschops]

I thought it was pretty simple, but obviously not clear enough. If you're going to change the pointer strdup gives you, you need to make a copy of it first, and call free on that copy.

pointer = strdup(something);
copy_of_pointer = pointer;  // HERE IS THE COPY. DO NOT CHANGE THE COPY
pointer++; // Do whatever you like with the pointer strdup gave you
free(copy_of_pointer); // HERE IS THE FREE. 

To answer your other question, STRDUP DOESN'T CALL FREE FOR YOU. YOU HAVE TO DO IT YOURSELF.

As an aside, the posts above already answered these questions very clearly. If you didn't understand, then I suspect you're not ready for handling your own memory yet. You need to go back to basics and learn about memory and pointers. You may think you already understand them, but it's clear that you don't, and without understanding memory and pointers, you're going to have a lot of trouble.

[can-mohan]

dear moschop thanks for your explanation. same thing i want to tell u that it is not worth to use strdup to save the two lines of the code as anyway you are going to call free with strdup and it is not going to job similar to smart pointer (c++) :) . below pointers will throw some light why to not use strdup function.

1.It's not strictly ANSI C, but rather POSIX. Consequently, some compilers (e.g. MSVC) discourage use (MSVC prefers _strdup), and technically the C standard could define its own strdup with different semantics since str is a reserved prefix. So, there are some potential portability concerns with its use.
2.It hides its memory allocation. Most other str functions don't allocate memory, so users might be misled (as you say) into believing the returned string doesn't need to be freed.

3.Blindly duplicating strings all over the place rather than using them in-place when possible wastes time and memory and introduces failure cases into code that might otherwise be failure-free.

4.When you do need a copy of a string, it's likely you actually need more space to modify or build on it, and strdup does not give you that.

5.majority of the concern about strdup comes from security concerns regarding buffer over runs, and improperly formatted strings.

6.If a non-null terminated string is passed to strdup it can allocated an undefined length string.

7.•the main one being it hides the allocation. The other str* functions and most other standard functions require no free afterwards, so strdup looks innocuous enough and you can forget to clean up after it.