Fix memory leak

Question

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?

`strcat(tmp, section)` assumes `tmp` holds a string. It does not. — ouah, Nov 09 '12 at 18:18
FYI: http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc - just saw this, was news to me! — weston, Nov 09 '12 at 20:57

score 1 · Answer 1 · answered Nov 09 '12 at 18:21

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.

Piotr Zierhoffer · Answer 2 · 2012-11-09T21:07:41.823

1

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.

edited Nov 09 '12 at 21:07

answered Nov 09 '12 at 18:24

Piotr Zierhoffer

5,005
1
38
59

That's a good point, indeed. Maybe then the function can return `void`, or some value to indicate something failed (the call to `malloc`, for instance). – Fabien Nov 09 '12 at 18:26
Of course it should return void, copy-paste error, now fixed! Thanx @Fabien! – Piotr Zierhoffer Nov 09 '12 at 21:07

weston · Accepted Answer · 2012-11-09T20:59:34.047

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

score 0 · Answer 4 · answered Nov 09 '12 at 18:19

0

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.

answered Nov 09 '12 at 18:19

Omkant

9,018
8
39
59

score 0 · Answer 5 · answered Nov 09 '12 at 18:23

0

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.

answered Nov 09 '12 at 18:23

Ed Heal

59,252
17
87
127

Fix memory leak

5 Answers5