Freeing a dynamically allocated structure instance in C

Question

I have following structure:

typedef struct generic_attribute_struct{
    attribute_value current_value;
    attribute_value previous_value;
    attribute_value running_value;
} generic_attribute;

where attribute_value is simply a placeholder for unsigned long long int.

I have the following constructor for this structure:

generic_attribute* construct_generic_attribute(attribute_value current_value){
    generic_attribute *ga_ptr;                              //allocate space for pointer to generic attribute
    if(ga_ptr = malloc (sizeof (generic_attribute))){       //if allocation succeeds
        set_ga_current_value(ga_ptr, current_value);        //assigned the current value to given input
        set_ga_previous_value(ga_ptr, 0);                   //set previous value to zero
        set_ga_running_value(ga_ptr);
    } else{                                                 //else, inform user of error
        fprintf(stderr, "Health Monitor ran out of space when allocating memory for a generic attribute.");
    }
    return ga_ptr; // return pointer to new attribute or a NULL pointer if memory allocation failed
}

where set_ga_running_value looks like this:

attribute_value set_ga_running_value(generic_attribute* ga_ptr){
    attribute_value delta = get_ga_current_value(ga_ptr) - get_ga_previous_value(ga_ptr);
    ga_ptr->running_value = (ga_ptr->running_value? ga_ptr->running_value : 0) + delta;
    return ga_ptr->running_value;
}

Destructor for this structure looks like this:

void destroy_generic_attribute(generic_attribute** ga_ptr){
    free(*ga_ptr);
}

I created a test that asks the user for some current_value input, constructs a pointer, and prints out whether the values for structure variables are what they ought to be. This test loops until the user no longer wants to test, in which case they quit out of the test.

So, the test looks like this:

1. Does user want to test? If yes, go to 2). If no, go to 7).
2. Get input from user
3. Call constructor for generic attribute with this new input
4. Verify that generic attribute was created correctly
5. Call the destructor on generic attribute
6. Go to 1.
7. Quit

This is what the test looks like:

void test_generic_attribute_constructor_with_user_input(){
    attribute_value input;
    int continue_var;
    bool current_value_test, previous_value_test, running_value_test, pointer_test;
    generic_attribute* ga_ptr;
    while(1){
        printf("Would you like to execute a test for Generic Attribute Constructor? Type 1 for YES, or 0 for NO: ");
        scanf("%i", &continue_var);
        if(continue_var){
            printf(TESTING, "Constructor for Generic Attribute and Generic Attribute Reader");
            printf("\n" INPUT, "single number");
            scanf("%lld", &input);
            ga_ptr = construct_generic_attribute(input);
            read_generic_attribute(ga_ptr);
            current_value_test = (get_ga_current_value(ga_ptr) == input ? true : false);
            previous_value_test = (get_ga_previous_value(ga_ptr) == 0 ? true: false);
            // THIS TEST FAILS
            running_value_test = (get_ga_running_value(ga_ptr) == input ? true: false);
            pointer_test = (ga_ptr ? true: false);
            printf("%s.\n", ((current_value_test && previous_value_test && running_value_test && pointer_test) ? PASS : FAIL));
            destroy_generic_attribute(&ga_ptr);

        }else{
            printf("\nOK! Testing concluded.");
            break;
        }

    }
}

My problem is that the "running value" seems to never get "reset" when the ga_ptr gets destroyed. It seems to keep its old value. How would I clear memory for the entire ga_ptr structure properly?

test result:

Would you like to execute a test for Generic Attribute Constructor? Type 1 for YES, or 0 for NO: 1
Testing Constructor for Generic Attribute and Generic Attribute Reader:
Please enter single number for input: 10
Generic Attribute has the following contents:

             Pointer       Current Value       Previos Value       Running Value
         0x600058530                  10                   0                  10
PASS.
Would you like to execute a test for Generic Attribute Constructor? Type 1 for YES, or 0 for NO: 1
Testing Constructor for Generic Attribute and Generic Attribute Reader:
Please enter single number for input: 20
Generic Attribute has the following contents:

             Pointer       Current Value       Previos Value       Running Value
         0x600058530                  20                   0                  30
FAIL.

I would expect the Running value to be 20.

If I change the destructor to this:

void destroy_generic_attribute(generic_attribute** ga_ptr){
    set_ga_current_value(*ga_ptr, 0);
    set_ga_previous_value(*ga_ptr, 0);
    (*ga_ptr)->running_value = 0;
    free(*ga_ptr);
}

My test pass... However, I do not understand why skipping the setters makes the code fail.

Answer 1

You are just invoking undefined behaviour by using a never initialized value.

In construct_generic_attribute, you initialize current and previous value and then call set_ga_running_value. In the latter, you use current and previous values that have just been initialize to compute delta : fine until here. But then you have :

ga_ptr->running_value = (ga_ptr->running_value? ga_ptr->running_value : 0) + delta;

Meaning that you use running_value before initializing it. As it is in a freshly malloc'ed struct, its value is just undefined. It might be 0, or it might be the value that existed in this memory location before allocation, or it might be a special value that compiler uses as a special initialization : you cannot know and should not rely on anything.

You compiler seems to preinitialize memory to 0 on run and then never change the value on free and malloc, giving 30 on second run. Mine (in debug mode) allways initialized malloc'ed values to 0xcdcdcdcd, giving FAIL on each test. Just undefined behaviour ...

So you really should initialize the freshed allocated struct in your constructor :

if(ga_ptr = malloc (sizeof (generic_attribute))){       //if allocation succeeds
    memset(ga_ptr, 0, sizeof(generic_attribute));       // ensure all values are set to 0
    set_ga_current_value(ga_ptr, current_value);        //assigned the current value to given input
    set_ga_previous_value(ga_ptr, 0);                   //set previous value to zero
    set_ga_running_value(ga_ptr);

or if you know that running_value is an integral, just replace the memset with:

ga_ptr->running_value = 0;