CODEWITHSUNDEEP
cmemorypointers
user308827
user308827

Reputation: 22031

Fix memory leak

I have the following function which gets called multiple times in my code:

char* get_section_name(const char* section, const char* value) {

  char *tmp = (char *)malloc(STR_LEN * sizeof(char));

  if(strlen(section)>0) {

      strcat(tmp, section);

      strcat(tmp,".");

  }

  strcat(tmp, value);    

  return tmp;

}

and I call it in other functions like this:

section_name = get_section_name(data->model_name,"params_default");

What is the best way to free this memory? Can I just call free(section_name) when I am done?

Upvotes: 1

Views: 132

Answers (5)

Piotr Zierhoffer
Piotr Zierhoffer

Reputation: 5151

free would be great here, but as an alternative, might be the best way to do this would be a change to signature. If you make it like 

void get_section_name(const char* section, const char* value, char * result)

then you can pass a pointer to an allocated memory, so the user of this function is perfectly aware of how should he handle the memory after it's used.

Upvotes: 1

weston
weston

Reputation: 54811

Yes free, however, you could consider a different name that makes it clear it is allocating memory. Also, you could make use of sprintf for combining 2 strings and strdup for just copying one.

char* create_section_name(const char* section, const char* value) { 
  const int sectionLen = strlen(section);   

  if(sectionLen>0) {
      //+1 for the period and +1 for the null
      char *tmp = (char *)malloc((sectionLen + strlen(value) + 2) * sizeof(char));
      //do that often? consider a newString(size) macro. See below
      sprintf(tmp, "%s.%s", section, value);    
      return tmp;
  }
  else {    
    return strdup(value);
  }
}

This assumes you don't need the full STR_LEN, it uses just enough in both cases.

newString macro I suggested:

#define newString(size) (malloc((size) * sizeof(char)))

Or it can even automatically add one for the null:

#define newString(size) (malloc(((size)+1) * sizeof(char)))

Then malloc is replaced with this:

char *tmp = newString(sectionLen + strlen(value) + 1); //+1 for period

Upvotes: 1

Ed Heal
Ed Heal

Reputation: 60037

I am going to make a leap of faith that STR_LEN is going to be of sufficient size. If so then free(section_name); will suffice. Bur use strcpy instead of strcat or initialize a null string.

Upvotes: 0

Fabien
Fabien

Reputation: 13446

First, you must make sure tmpwas actually allocated (ie mallocdid not fail) :

tmp = (char *)malloc(STR_LEN * sizeof(char));
if (tmp == NULL) {
  // quit now !
}

Then, as you strcat it, you must be sure tmp is an empty string, ie its first character is 0

tmp[0] = '\0';

Then, yes, you can free it the way you wrote it.

One last thing : you have to be sure that strlen(section)+strlen(".")+strlen(value) < STR_LEN, or you will overwrite memory you're not supposed to.

Upvotes: 1

Omkant
Omkant

Reputation: 9234

Always perform error check when creating memory using malloc(),calloc(), orrealloc()

Yes you can use free here

free(section_name) , Because tmpreturned is stored in section_name which now points to malloced memory.

Upvotes: 0

Related Questions