Deallocated memory functions C

Question

I have problems with memory deallocation in C. Without the division of functions everything is OK, but unfortunately it does not work on the same functions. Here is the code:

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

typedef struct {
    char *name;
    enum {
        summer,
        winter
    } index;
} student;

bool init(student *s) {
    printf("The next stage in the allocation of memory\n");
    s = (student*)malloc(sizeof(*s));
    if (&s == NULL) {
        printf("Allocation Failed\n");
        return 0;
    } else {
        printf("Allocation completed successfully\n");
    }
}   

void delete(student *s) {
    if (&s != NULL) {
        printf("begin removal\n");
        free(s);
        printf("Released memory");
    }
}

int main() {
    student *s;
    init(s);
    delete(s);

    return 0;
}

I do not know what I'm doing wrong. Please help.

Answer 1

First of all the function init has undefined bbehaviour because it returns nothing in the case of successful memory allocation.

You can check whether the memory was allocated or not by returning pointer to the allocated memory or NULL.

Also this statement

if(&s==NULL){

is wrong. The condition will always yield false because the address of the local variable s is not equal to NULL.

So the function can be rewritten either the following way

student * init()
{
    printf("The next stage in the allocation of memory\n");

    student *s = ( student* )malloc( sizeof( *s ) );

    if ( s == NULL )
    {
        printf("Allocation Failed\n");
    } 
    else 
    {
        printf("Allocation completed successfully\n");
    }

    return s;
} 

And called like

int main( void )
          ^^^^^
{
    student *s = init();
    //...

Or it can be defined the following way

int init( student **s )
{
    printf("The next stage in the allocation of memory\n");

    *s = ( student* )malloc( sizeof( **s ) );

    int success = *s != NULL;

    if ( !success )
    {
        printf("Allocation Failed\n");
    } 
    else 
    {
        printf("Allocation completed successfully\n");
    }

    return success;
} 

and called like

int main( void )
          ^^^^^
{
    student *s;
    init( &s );
    //...

The function delete should be defined at least like

void delete(student *s) {
    if (s != NULL) {
       ^^^
        printf("begin removal\n");
        free(s);
        printf("Released memory");
    }
}

Answer 2

Firstly, free is NULL safe. If variable is NULL already, basically nothing happens. You do not have to check if it is NULL. (you can check page 313 ISO-IEC 9899 )

Also, when you initialize student->name and allocate, there will be memory leak. You have to free that too.

So, your delete function could be like this ;

  void delete(student *s) {
    printf("begin removal\n");
    free(s->name);
    free(s);
    printf("Released memory");
}

And if (&s == NULL) is wrong. They must be changed with if (s == NULL).

Your allocation may cause really big troubles in the big codes. If you allocate s = (student*)malloc(sizeof(*s)); it means that "allocate s with size of *s". But pointer size is fixed memory block (mostly 8 bytes). It means that you blocks certain size of memory. If you have bigger struct than that, this kind of allocation will corrupt the memory and your executable will be killed by the OS(you can try add some more variables to your struct and initialize them). In small structs and very short runtimes, mostly this allocation works too. But i guarantee that this is not safe for run-time. And it will not give any warnings or errors in compile-time. True way is s = malloc(sizeof(student)). With this way, you exactly allocates all the memory blocks. And your memory stay safe in run-time.

Lastly, your init function should return the initialized variable. And your init function could be like this ;

#define NAME_LENGHT 128

...

student * init(student *s) {
   printf("The next stage in the allocation of memory\n");
   s = malloc(sizeof(student));
   if (s == NULL) {
       printf("Allocation Failed\n");
       return NULL;
   }

   s->name = malloc(NAME_LENGHT); 
   if (s->name == NULL) {
       printf("Allocation Failed\n");
       return NULL;
   } else {
       printf("Allocation completed successfully\n");
   }
   //alternatively you can strdup directly without any allocation
   // s->name = strdup("some name");
   return s; 
}   

Answer 3

You never change the s in main.

Solution 1: Pass a pointer to the variable in main for init to populate.

void init_student(student** s_ptr) {
    *s_ptr = (student*)malloc(sizeof(student));
    if (*s_ptr == NULL) {
        fprintf(stderr "panic: Allocation Failed\n");
        exit(1);
    }

    (*s_ptr)->name = malloc(MAX_NAME_SIZE + 1);
    if ((*s_ptr)->name == NULL) {
        fprintf(stderr "panic: Allocation Failed\n");
        exit(1);
    }

    (*s_ptr)->name[0] = 0;

    (*s_ptr)->gpa = 0;
}   

void delete_student(student* s) {
    free(s->name);
    free(s);
}

int main() {
    student* s;
    init_student(&s);
    delete_student(s);
    return 0;
}

Solution 1b: Same, but a cleaner implementation.

void init_student(student** s_ptr) {
    student* s = (student*)malloc(sizeof(student));
    if (*s_ptr == NULL) {
        fprintf(stderr "panic: Allocation Failed\n");
        exit(1);
    }

    s->name = malloc(MAX_NAME_SIZE + 1);
    if (s->name == NULL) {
        fprintf(stderr "panic: Allocation Failed\n");
        exit(1);
    }

    s->name[0] = 0;

    s->gpa = 0;

    *s_ptr = s;
}   

void delete_student(student* s) {
    free(s->name);
    free(s);
}

int main() {
    student* s;
    init_student(&s);
    delete_student(s);
    return 0;
}

Solution 2: Return the allocated point to main.

student* init_student() {
    student* s = (student*)malloc(sizeof(student));
    if (s == NULL) {
        fprintf(stderr "panic: Allocation Failed\n");
        exit(1);
    }

    s->name = malloc(MAX_NAME_SIZE + 1);
    if (s->name == NULL) {
        fprintf(stderr "panic: Allocation Failed\n");
        exit(1);
    }

    s->name[0] = 0;

    s->gpa = 0;

    return s;
}   

void delete_student(student* s) {
    free(s->name);
    free(s);
}

int main() {
    student* s = init_student();
    delete_student(s);
    return 0;
}

---

Note that &s == NULL will never be true because &s is the address of the variable itself. You want just s == NULL to check the value of the pointer in s.