C free char* allocated on heap

Question

Is there a memory leak in the following code example as I have allocated memory on the heap for name which hasn't been freed? If I add free(person->name); before the free(person); line then I get a runtime error in VS "CRT detected that the application wrote to memory after end of heap buffer".

header.h:

#ifndef HEADER
#define HEADER

typedef struct person {
 char *name;
} Person;

#endif

source.c:

#include <stdio.h>
#include <stdlib.h>
#include "header.h"

#define BUFFERSIZE 500

int main (int argc, char* argv[]){
 Person *person = NULL;
 char buffer[BUFFERSIZE];
 printf("Enter name\n");
 fgets(buffer, BUFFERSIZE, stdin);
 {
  char *name; 
  person = malloc(sizeof(Person));
  name = malloc(strlen(buffer));
  strcpy (name,buffer);
  person->name = name;
 }
 free(person); 
 return 0;
}

Thanks, Rob

Solution 1

Yes, the code has a memory leak.

The culprit is: name = malloc(strlen(buffer)); which should be
name = malloc(strlen(buffer)+1); to account for \0.

You should now be able to free person->name.

Solution 2

strlen doesn't account for the null-terminator that strcpy adds, you'll need to "malloc(strlen(buffer)+1)"

Solution 3

Yes. There is a memory leak.

You allocate a name buffer:

name = malloc(strlen(buffer));

and never deallocate it. Then when you deallocate the Person,

free(person); 

you lose your last pointer to that buffer, so you can never deallocate it.

The error message you are getting is a separate issue related to the length of the name buffer that you allocate being insufficient to include the null termination of the string you try to copy into it.

Solution 4

name is never freed, so it leaks.

Complex heap-allocated structures like this are often best handled by a move to C++, or (if that isn't feasible) by wrapping the memory management in custom create/destroy/manipulation functions:

Person *CreatePerson(void) {
    /* you can use calloc() here, of course */
    Person *result = malloc(sizeof(Person));
    if (result) {
        memset(result, 0, sizeof(Person));
    }
    return result;
}

void DestroyPerson(Person *p) {
    if (p) {
        free(p->name);
        free(p);
    }
}

void SetPersonName(Person *p, const char *name) {
    if (p) {
        if (p->name) {
            free(p->name);
            p->name = NULL;
        }

        if (name) {
            size_t len = strlen(name);
            p->name = malloc(len + 1);
            if (p->name) {
                strncpy(p->name, name, len);
                p->name[len] = 0;
            }
        }
    }
}

const char *GetPersonName(const Person *p) {
    if (p)
        return p->name;
    return NULL;
}

However, as you can see by the amount of code involved, C++ is often the better choice:

class Person {
    private: std::string name;

    public: const std::string& getName(void) const {
        return this->name;
    }

    public: void setName(const std::string& newName) {
        this->name = newName;
    }
};

Much shorter, cleaner, and clearer!

View more solutions

Comments

[ruslik]

2 allocs vs 1 free. What do you expect?

[Thomas L Holaday]

Compare: mkdir person; mkdir person\name; deltree /Y person. Two resource allocations, one resource release. free doesn't work that way, and now he knows.

[Chris Becke]

You can call strdup, for a one pass, malloc+1 and copy on a string operation.

[Jonathan Grynspan]

strdup() is, however, nonstandard C. :)