malloc issues- C program runs in debugging but crashes in normal running

Question

EDIT: SOLVED. THANKS!

I have a function that returns a pointer to a struct I built. I have a problem that my code runs in debugging mode but crashes in normal mode, It crashes only when I'm trying to add kids to the person- starts at: if (kidsNumber>0){ I'm pretty sure it's initializing issue but I can't find where and I'm stuck. I tried to put flags and breakpoint and nothing helped.. Thank you!

Person* CreatePerson(){
    Person* newPerson;
    newPerson=(Person*)malloc(sizeof(newPerson));
    if (newPerson==NULL){
        return NULL;
    }

    InitPersonValues(newPerson);
    char tempName[MAX_NAME];
    int id;
    int kidsNumber;
    printf("Name:\n");
    scanf("%s",tempName);
    newPerson->name=(char*)malloc(1 + strlen(tempName));
    if (newPerson->name == NULL){
        return NULL;
    }
    strcpy(newPerson->name,tempName);

    printf("ID:\n");
    scanf("%d", &id);
    newPerson->id=id;

    printf("Num of kids:\n");
    scanf("%d",&kidsNumber);
    newPerson->numOfKids=kidsNumber;
    if (kidsNumber>0){
        newPerson->kids=malloc(kidsNumber*sizeof(char*));
        if (newPerson->kids==NULL){
            return NULL;
        }
        int i=0;
        for (i=0;i<kidsNumber;i++){
            strcpy(tempName,"");
            printf("Kid #%d name:\n",i+1);
            scanf("%s",tempName);
            newPerson->kids[i]=malloc(strlen(tempName)+1);
            if (newPerson->kids[i]==NULL){
                return NULL;
            }
            // printf("%s\n",tempName);
            strcpy(newPerson->kids[i],tempName);
            printf("%s\n",newPerson->kids[i]);
        }
    }
    return newPerson;
}

Answer 1

This code doesn't allocate enough memory for the terminating NUL character of the string copied:

newPerson->name=(char*)malloc(strlen(tempName)*sizeof(char));

A string contains strlen() char values, plus the terminating NUL.

Change that line to read

newPerson->name=malloc(1 + strlen(tempName));

Note that sizeof(char) is one by definition. And you do not need to cast the result of malloc() in C.

Or, if it's available, you can simply use strdup(). It's not standard C because it violates an unwritten "rule" regarding implicit allocation of memory that needs to be free()'d, but it works.

Also, as noted in the comments:

newPerson=(Person*)malloc(sizeof(newPerson));

is wrong as it allocates a block of memory the size of newPerson, a pointer, instead of the struct. Change it to

newPerson=malloc(sizeof(*newPerson));

Answer 2

Please don't cast malloc and other functions of it's family. See Do I cast the result of malloc?

The best way of using a malloc is:

int *var = malloc(size * sizeof *var);

Avoid using sizeof(int*) in the arguments. It's easy to make mistakes, to forget a *, etc. If you later change the data type, you have to change the arguments as well. sizeof *var returns the correct size every time, regardless of the type (as long as var is a pointer).

Same goes for calloc

int *var = calloc(size, sizeof *var);

You see the problem?

newPerson->kids=(char**)calloc(kidsNumber,sizeof(char));

doesn't allocate enough space, it's easy to miss the * in the size argument for calloc.

newPerson->kids=calloc(kidsNumber, sizeof *newPerson->kids);

returns you the exact amount of memory.

Also,

newPerson->name=(char*)malloc(strlen(tempName)*sizeof(char));

Do not cast, don't use sizeof(char)

newPerson->name=malloc(strlen(tempName) + 1); // for strings

// or if you want to be 100% correct, but that's overkill
// since you know that you are requesting space for a C-String
newPerson->name=malloc((strlen(tempName) + 1) * sizeof *newPerson->name);

If possible, also use strdup if you want to clone a string. If you're environment doesn't have strdup, write one yourself:

char *strdup(const char *s)
{
    char *str = malloc(strlen(s) + 1);
    if(str == NULL)
        return NULL;
    strcpy(str, s);
    return str;
}

That will make your code more readable.

---

EDIT

One minor thing I noticed after re-reading your question.

If you notice that while creating a new Person something fails, don't just return NULL and be done with it. Free the memory you've already allocated.

I'd do something like this:

void DestroyPerson(Person *person)
{
    if(person == NULL)
        return;

    if(person->kids)
    {
        int i;
        for(i = 0; i < person->numOfKids; ++i)
            free(person->kids[i]);

        free(person->kids);
    }

    free(person);
}

// returns 1 on success, 0 otherwise
int InitPerson(Person *person)
{
    memset(person, 0, sizeof *person);

    // some other initializations
    ...
    return 1;
}

Person* CreatePerson() {
    Person *newPerson;

    newPerson=malloc(sizeof *newPerson);
    if (newPerson==NULL){
        return NULL;
    }

    // initializing kids
    if(!InitPerson(person))
    {
        DestroyPerson(person);
        return NULL;
    }

    ...

    if(some_error_detected)
    {
        DestroyPerson(newPerson);
        return NULL;
    }

    ...
}