Valgrind Invalid read of size 4 with linked list in C

Question

I encountered some problems with my linked list in C. I'm actually doing a project using sockets connections with a server and clients. I decided to use a linked list for my clients. This is my structure :

typedef struct ClientsList_s {
    int sockfd;
    struct ClientsList_s *next;
} ClientsList_t;

So, when I execute my work with valgrind, I got 6 errors from 6 context. Here is an example of one error:

Invalid read of size 4
==33211==    at 0x109531: get_max_socket (server.c:26)
==33211==    by 0x1097C2: do_server_loop (server.c:80)
==33211==    by 0x109A40: launch_teams (main.c:53)
==33211==    by 0x109AE6: main (main.c:63)
==33211==  Address 0x4a58040 is 0 bytes inside a block of size 16 free'd
==33211==    at 0x483CA3F: free (in /usr/lib/x86_64-linux- 
gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==33211==    by 0x109675: receive_socket_message (server.c:47)
==33211==    by 0x109832: do_server_loop (server.c:86)
==33211==    by 0x109A40: launch_teams (main.c:53)
==33211==    by 0x109AE6: main (main.c:63)
==33211==  Block was alloc'd at
==33211==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux- 
gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==33211==    by 0x10937B: add_client (client.c:18)
==33211==    by 0x109777: check_new_socket (server.c:68)
==33211==    by 0x109816: do_server_loop (server.c:84)
==33211==    by 0x109A40: launch_teams (main.c:53)
==33211==    by 0x109AE6: main (main.c:63)

This is the function where the error come from:

static int get_max_socket(MasterSocket_t *server, ClientsList_t **clients)
{
    int maxSd = server->sockfd;
    ClientsList_t *temp = *clients;

    FD_ZERO(&server->readfds);
    FD_SET(server->sockfd, &server->readfds);
    while (temp != NULL) {
        if (temp->sockfd > 0)
            FD_SET(temp->sockfd, &server->readfds);
        if (temp->sockfd > maxSd)
            maxSd = temp->sockfd;
        temp = temp->next;
    }
    return (maxSd);
}

So all the errors appear when I close a client and the server is still running. Every line where I use the temp in the while give me an error. So first I thought it was an initialization problem but it doesn't seem to be that. Here is my function:

void add_client(ClientsList_t **clients, int newSocket, int port)
{
    ClientsList_t *temp = NULL;
    ClientsList_t *last = *clients;

    temp = malloc(sizeof(ClientsList_t));
    temp->sockfd = newSocket;
    temp->next = NULL;
    if (*clients == NULL)
        *clients = temp;
    else {
        while (last->next != NULL)
            last = last->next;
        last->next = temp;
    }
}

And there is the function where I destroy an element when i close his client :

static void receive_socket_message(ClientsList_t **clients,
                                   MasterSocket_t *server)
{
    ClientsList_t *temp = *clients;
    ClientsList_t *tempDel = NULL;
    int valread = 0;
    int addrlen = 0;
    char buffer[1025];

    while (temp != NULL) {
        if (FD_ISSET(temp->sockfd, &server->readfds)) {
            if ((valread = read(temp->sockfd, buffer, sizeof(buffer))) == 0) {
                tempDel = temp;
                temp = temp->next;
                close(tempDel->sockfd);
                free(tempDel);
                continue;
            } else {
                buffer[valread] = '\0';
                printf("%s\n", buffer);
            }
        }
        temp = temp->next;
    }
}

And in case you wanna test something, there is my loop function :

static int check_new_socket(ClientsList_t **clients,
                            MasterSocket_t *server, int port)
{
    int newSocket = 0;

    if (FD_ISSET(server->sockfd, &server->readfds)) {
        if ((newSocket = accept(server->sockfd,
                                (struct sockaddr *)&server->address,
                                (socklen_t *)&server->addrlen)) < 0)
            return (-1);
        add_client(clients, newSocket, port);
    }
    return (0);
}

int do_server_loop(MasterSocket_t server, int port)
{
    int maxSd = 0;
    int activity = 0;
    ClientsList_t *clients = NULL;

    while (true) {
        maxSd = get_max_socket(&server, &clients);
        if ((activity = select(maxSd + 1, &server.readfds,
                               NULL, NULL, NULL)) < 0 
        &&  (errno != EINTR))
            return (-1);
        if (check_new_socket(&clients, &server, port) == -1)
            return (-1);
        receive_socket_message(&clients, &server);
    }
    destroy_clients(clients);
    return (0);
}

Thanks for you help.

Answer 1

The problem is in receive_socket_message: when you free a list element, you do not link the previous node to the next node.

Here is a modified version:

static void receive_socket_message(ClientsList_t **clients,
                                   MasterSocket_t *server)
{
    ClientsList_t *temp = *clients;
    ClientsList_t *tempDel = NULL;
    ClientsList_t **link = clients;
    int valread = 0;
    int addrlen = 0;
    char buffer[1025];

    while (temp != NULL) {
        if (FD_ISSET(temp->sockfd, &server->readfds)) {
            if ((valread = read(temp->sockfd, buffer, sizeof(buffer))) == 0) {
                tempDel = temp;
                *link = temp = temp->next;
                close(tempDel->sockfd);
                free(tempDel);
                continue;
            } else {
                buffer[valread] = '\0';
                printf("%s\n", buffer);
            }
        }
        link = &temp->next;
        temp = temp->next;
    }
}