using free() function causes runtime error

Question

I have a struct called Person, that contains two attributes - first and last name. After successfully dynamic allocation of memory for a variable of Person type, giving values to the attributes I would like to free the memory, but I keep getting a runtime error (the program window just crashes) this it the code:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

typedef struct {
char firstName[15];
char lastName[15];
} Person;

void main(){
int len = 0;
char  firstName[]="danny", lastName[]="johnes";

Person *temp = (Person*)malloc(sizeof(Person));
if (temp == NULL)
    return;

    len = strlen(firstName);
    temp->firstName[len] = (char*)malloc(sizeof(char)*(len));
    if (temp->firstName == NULL)
        return;
    strcpy(temp->firstName, firstName);

    len = strlen(lastName);
    temp->lastName[len] = (char*)malloc(sizeof(char)*(len));
    if (temp->firstName == NULL)
        return;
    strcpy(temp->lastName, lastName);

freePerson(temp);
system("pause");
return;
}

This is the function I use to free the memory:

void freePerson(Person* ps) {
    if (ps != NULL) {
        free(ps->firstName);
        free(ps->lastName);
        free(ps);
    }
}

All I want the code to do - is to store the name in a dynamically allocated structure, and free it. Later on, I plan to replace the hard-coded names with values inputed from file.

Any ideas about the error? Thank you.

[don't cast malloc](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc) — Barmar, Jan 04 '19 at 22:58
You are trying to free fixed sized arrays inside the Person structure. They are not pointers to dynamically allocated arrays. Am I wrong? — NthDeveloper, Jan 04 '19 at 23:03

chux - Reinstate Monica · Answer 1 · 2019-01-04T23:26:56.963

At least 5 issues:

To duplicate a string, insure allocation includes enough room for the characters including the null character.

Otherwise the strcpy() writes outside the allocation which is undefined behavior (UB).

    len = strlen(firstName);
    // temp->firstName[len] = (char*)malloc(sizeof(char)*(len ));
    temp->firstName = (char*)malloc(sizeof(char)*(len + 1));
    //                                                + 1 
    ...
    strcpy(temp->firstName, firstName);

Same for lastName.

Also assign to the pointer, not the char. @Barmar

Person members are arrays. For dynamic allocation, they should be pointers. @NthDeveloper

typedef struct {
  // char firstName[15];
  // char lastName[15];
  char *firstName;
  char *lastName;
} Person;

2nd test is wrong

// if (temp->firstName == NULL)
if (temp->lastName == NULL)

int vs. size_t.

int len = 0; assumes the string length fits in a int. Although this is exceedingly common, the type returned from strlen() is size_t. That unsigned type is right-sized for array indexing and sizing - not too wide, not too narrow. Not a key issue in this learner code.

// int len = 0;
size_t len = 0;

Tip: cast not needed. Allocate to the referenced object, not the type. Easier to code right, review and maintain.

// Person *temp = (Person*)malloc(sizeof(Person));
Person *temp = malloc(sizeof *temp);

// temp->firstName[len] = (char*)malloc(sizeof(char)*(len + 1));
temp->firstName = malloc(sizeof *(temp->firstName) * (len + 1));

Tip: Although not C standard, many platforms provide strdup() to allocated and copy strings. Sample strdup() code.

temp->firstName = strdup(firstName);

Tip: Likely the most valuable one: A good compiler with warnings well enabled should have warned about temp->firstName[len] = (char*)malloc(sizeof(char)*(len)); as it is a questionable type mis-match in the assignment. These warnings save you and us all time. Insure your next compilation has all warning enabled.

score 1 · Answer 2 · answered Jan 04 '19 at 23:04

You have already space allocated for firstName, so you have to copy the name within the size constraits (15 bytes). You can do this best with snprintf like this:

snprintf(temp->firstName, sizeof(temp->firstName), "%s", firstName);

Same goes for lastName. Mind that both might be truncated if the length exceeds the size of the field.

The other option is to allocate the fields dynamically. Then your struct members should be pointers, not char arrays:

typedef struct {
    char *firstName;
    char *lastName;
} Person;

You can then allocate and assign the names like this:

temp->firstName = strdup(firstName); // (same for lastName)

But mind that you have to free these fields seperately if you want to free the whole item.

Nice use of `snprintf(temp->firstName, sizeof(temp->firstName), "%s", firstName);` over `strncpy()` as your code insures `temp->firstName` is _null character_ terminated - regardless of its prior state. I'd expect a good compiler to emit efficient code even though the source code appears ponderous. UV — chux - Reinstate Monica, Jan 04 '19 at 23:36

score 1 · Answer 3 · answered Jan 04 '19 at 23:07

If you don't want to specify a maximum size for the names in the structure, you need to declare them as pointers, not arrays.

typedef struct {
    char *firstName;
    char *lastName;
} Person;

Then you should assign the result of malloc() to the member, without indexing it. You also need to add 1 to strlen(firstName), to make space for the null terminator.

temp->firstName = malloc(strlen(firstName)+1);
if (temp->firstName == NULL) {
    return;
}
strcpy(temp->firstName, firstName);

I still think, that `strdup()` is the cleaner and better readable way to duplicate a string (and probably performs even better, too). — Ctx, Jan 04 '19 at 23:15

score 1 · Answer 4 · answered Jan 04 '19 at 23:15

This is how I would write this:

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

#define FIRSTNAME_MAXLEN 15
#define LASTNAME_MAXLEN 15

typedef struct
{
        char firstName[FIRSTNAME_MAXLEN+1];
        char lastName[LASTNAME_MAXLEN+1];
} person_t;

void freePerson(person_t *ps) {
        if (ps) {
                free(ps); ps=NULL;
        }
}

int main(){
        const char *firstName="danny";
        const char *lastName="johnes";

        person_t *temp = calloc(1, sizeof(person_t));
        if (!temp) return 1;

        strncpy(temp->firstName, firstName, FIRSTNAME_MAXLEN);
        strncpy(temp->lastName, lastName, LASTNAME_MAXLEN);

        printf("test: firstname: %s\n", temp->firstName);
        printf("test: lastname: %s\n", temp->lastName);
        freePerson(temp);

        return 0;
}

You allocate enough room on the heap and cleanup things with calloc(), then you copy your string with strncpy() limiting to the bytes reserved and avoiding buffer overflow. At the end you need to free() the memory returned by calloc(). Since you allocated char firstName[] and char lastName[] inside your struct you don't need to reserve other memory with malloc() for those members, and you also don't need to free() them.

Instead of `if (ps) { free(ps); ...; }`, code can just use `free(ps);` as `free(NULL)` is OK. — chux - Reinstate Monica, Jan 04 '19 at 23:30
@chuk yes thanks, but this is a pattern I always use to avoid double free. In this way I can call freePerson() function more times without problems — dAm2K, Jan 04 '19 at 23:33
@dAm2K To avoid double-free the better measure is to set the pointer variable to NULL after freeing (as you suggest). No need to check if it is NULL beforehand. Of course, these measures might hide flawed logic in your code, which might fail on another point later then. — Ctx, Jan 04 '19 at 23:34

using free() function causes runtime error

4 Answers4