Valgrind error: Invalid write of size 8 while using lists made of struct and malloc

Question

I've problem with Valgrind, today it's the first day I started use it and I really not pratic with it. Here's my code:

//list' typedef
typedef struct nodo_s {
    int n;
    struct nodo_s * next;
} nodo_t;


//function with error
void cambiaDirezioneSinistra(nodo_t * head)
{
    nodo_t * tmp, *temp;
    for(tmp = head; tmp != NULL; tmp = tmp->next)
        ;
    printf("1s: %d\n", tmp->n);
    temp = head;
    printf("2s: %d\n", temp->n);
    head = temp->next;
    printf("3s: %d\n", head->n);
    tmp->next = temp; ----> error
    printf("4s: %d\n", tmp->n);
    temp->next = NULL;
}


//main
int main()
{
    int i;
    int dir;
    nodo_t * head = NULL;

    for(i = 0; i < N; i ++)
        head = nuovoNodo(head);

    stampaLista(head);

    printf("Inserisci la direzione di scambio, 0 per sinistra e 1 per destra.\n");
    scanf("%d", &dir);

    if(dir == 0)
        cambiaDirezioneSinistra(head);
    else if(dir == 1)
        cambiaDirezioneDestra(head);

    stampaLista(head);

    return 0;
}

And here's Valgrind report of the error:

 ==511== Invalid write of size 8
 ==511==    at 0x10930F: cambiaDirezioneSinistra (20200120_6.c:55)
 ==511==    by 0x1094EE: main (20200120_6.c:103)
 ==511==  Address 0x8 is not stack'd, malloc'd or (recently) free'd
 ==511== 
 ==511== 
 ==511== Process terminating with default action of signal 11 (SIGSEGV)
 ==511==  Access not within mapped region at address 0x8
 ==511==    at 0x10930F: cambiaDirezioneSinistra (20200120_6.c:55)
 ==511==    by 0x1094EE: main (20200120_6.c:103)

I've tried a lot of "solution" finded in internet, but I couldn't resolve it in any way. Thanks.

Answer 1

The function cambiaDirezioneSinistra has undefined behavior.

For starters the pointer to the head node is passed by value. So the function deals with a copy of the value of the pointer to the head node. Changing the copy does not influence on the original pointer.

void cambiaDirezioneSinistra(nodo_t * head)

The pointer to the head node can be equal to NULL. But there is no check within the function whether head is equal to NULL.

After this loop

for(tmp = head; tmp != NULL; tmp = tmp->next)
    ;

the pointer tmp will be equal to NULL. So accessing data members of the structure using this pointer does not make sense.

The function can be declared and defined the following way

//function with error
void cambiaDirezioneSinistra( nodo_t **head )
{
    if ( *head != NULL && ( *head )->next != NULL )
    {
        nodo_t *last = *head;

        while ( last->next ) last = last->next;

        last->next = *head;
        *head = ( *head )->next;

        last->next->next = NULL;
    }
}

And the function is called like

cambiaDirezioneSinistra( &head );

Here is a demonstrative program.

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

//list' typedef
typedef struct nodo_s 
{
    int n;
    struct nodo_s * next;
} nodo_t;


//function with error
void cambiaDirezioneSinistra( nodo_t **head )
{
    if ( *head != NULL && ( *head )->next != NULL )
    {
        nodo_t *last = *head;

        while ( last->next ) last = last->next;

        last->next = *head;
        *head = ( *head )->next;

        last->next->next = NULL;
    }
}

int append( nodo_t **head, int n )
{
    while ( *head ) head = &( *head )->next;
    
    *head = malloc( sizeof( nodo_t ) );
    int success = *head != NULL;
    
    if ( success )
    {
        ( *head )->n = n;
        ( *head )->next = NULL;
    }
    
    return success;
}

void display( const nodo_t *head )
{
    for ( ; head != NULL; head = head->next )
    {
        printf( "%d -> ", head->n );
    }
    
    puts( "null" );
}

void clear( nodo_t **head )
{
    while ( *head )
    {
        nodo_t *tmp = *head;
        *head = ( *head )->next;
        free( tmp );
    }
}

int main(void) 
{
    nodo_t *head = NULL;
    
    const int N = 10;
    
    for ( int i = 0; i < N; i++ ) append( &head, i );
    
    display( head );
    
    for ( int i = 0; i < N; i++ )
    {
        cambiaDirezioneSinistra( &head );
    
        display( head );
    }
    
    clear( &head );
    
    return 0;
}

Its output is

0 -> 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> 7 -> 8 -> 9 -> null
1 -> 2 -> 3 -> 4 -> 5 -> 6 -> 7 -> 8 -> 9 -> 0 -> null
2 -> 3 -> 4 -> 5 -> 6 -> 7 -> 8 -> 9 -> 0 -> 1 -> null
3 -> 4 -> 5 -> 6 -> 7 -> 8 -> 9 -> 0 -> 1 -> 2 -> null
4 -> 5 -> 6 -> 7 -> 8 -> 9 -> 0 -> 1 -> 2 -> 3 -> null
5 -> 6 -> 7 -> 8 -> 9 -> 0 -> 1 -> 2 -> 3 -> 4 -> null
6 -> 7 -> 8 -> 9 -> 0 -> 1 -> 2 -> 3 -> 4 -> 5 -> null
7 -> 8 -> 9 -> 0 -> 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> null
8 -> 9 -> 0 -> 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> 7 -> null
9 -> 0 -> 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> 7 -> 8 -> null
0 -> 1 -> 2 -> 3 -> 4 -> 5 -> 6 -> 7 -> 8 -> 9 -> null

Pay attention to that if you need to access the last node of the list in functions then it is better to define the singly-linked list as a two-sided singly-linked list.

For example

typedef struct lista_t
{
    nodo_t *head;
    nodo_t *tail;
} lista_t;