Null-check behaves incorrectly when iterating linked-list in C

Question

I am trying to learn C basics by writing a program which prompts for integers from user then stores the values inside a linked list. If an input is -128 or below, the current node is set to NULL and the prompt ends. The program then goes through the list and calculates the average of entered values. However, the counter of the list always ends up with one more element than expected and thus results in incorrect average, although I explicitly specify the NULL check before adding each value and increment counter.

The code is below:

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

int main()
{
    struct record {
        int temperature;
        struct record *next;
    };
    
    // Keep hold of the first record for iterating --v
    struct record *first = NULL;
    first = (struct record *)malloc(sizeof(struct record));
    
    // Keep hold of the current record used in the while loop --v
    struct record *current = first;
    
    while (true) {
        
        // Get user input --v
        int temp;
        printf("Enter measurement: ");
        scanf("%d", &temp);
        
        // If input is less than -128, end the loop --v
        if (temp <= -128) {
            current = NULL;
            break;
        }
        
        // Record the temperature --v
        current->temperature = temp;
        
        // Allocate memory for the next record --v
        current->next = (struct record *)malloc(sizeof(struct record));
        
        // The next now becomes the current because we are done with the current --v
        current = current->next;
        printf("Next record created!\n");
    }
    
    // Calculate average --v
    current = first;
    double sum = 0.;
    int count = 0;
    // As long as the record is not NULL --v
    while (current != NULL) {
        
        sum += current->temperature;
        count++;
        
        printf("%d %d\n", count, current->temperature);
        
        // Move to the next record --v
        current = current->next;
    }
    
    printf("\nAverage of the list values: %.1f", sum / count);
}

Is this correct behavior? Is there some mechanics in C that I am not aware of?

I put on some debugging line so that I can keep track of the counter as well as the corresponding record, and found out that the current property doesn't appear to be NULL although I explicitly set it to NULL. This is the output of the code:

Enter measurement: 22
Next record created!
Enter measurement: 22
Next record created!
Enter measurement: 22
Next record created!
Enter measurement: -128
1 22
2 22
3 22
4 0

Average of the list values: 16.5

I did try to free the memory of the current pointer by using free(current) but the result is not better since the temperature then just holds a random number.

Answer 1

The problem is that the pointers current and current->next are two different pointers that occupy different extents of memory. current is a local variable while current->next is a data member of a dynamically allocated node.

In this statement

    current = current->next;

you set the value of the pointer current->next to the pointer current. So the two pointers have the same value.

But within this if statement

    if (temp <= -128) {
        current = NULL;
        break;
    }
    

there is changed only the pointer current. The pointer "previous"current->next stays unchanged. That is the local variable current was changed but the data member next of the allocated dynamically node was not changed and points to a dynamically allocated node with uninitialized data members.

The approach like this when a memory is allocated

    // Allocate memory for the next record --v
    current->next = (struct record*) malloc(sizeof(struct record));
    
    // The next now becomes the current because we are done with the current --v
    current = current->next;

and its address is assigned to the pointer current->next and then there is an attempt to set the pointer to NULL within the loop in any case results in memory leak. You should redesign the logic of the code.

A simplest way is to use a pointer to pointer as for example

struct record *first = NULL;
struct record **current = &first;

while (true) {
    
    // Get user input --v
    int temp;
    printf("Enter measurement: ");
    scanf("%d", &temp);
    
    // If input is less than -128, end the loop --v
    if (temp <= -128) {
        break;
    }
    
    *current = malloc( sizeof( struct record ) );        

    ( *current )->temperature = temp;
    ( *current )->next = NULL; 

    current = &( *current )->next;  
  
    puts("Next record created!");
}

Outside the while loop you will need to use another name for a auxiliary pointer. For example

// Calculate average --v
struct record *tmp = first;
// and so on...

Pay attention to that -- you will need to free all the allocated memory when the list is not required any more.