Linked List in thread?

Question

I've written a C script where I attempt to receive NTP packets and add them to a linked list which then sorts it by number of NTP packets received per IP.

Using the checkInList() command seems to work fine in the test in main() but compares the wrong ip when it is called in recievethread().

Expected output:

[ (187.1.160.84:31) (255.255.255.255:400) (8.8.8.8:0) (192.168.2.1:0)  ]

Meaning that we received 400 packets from 255.255.255.255 0 from 8.8.8.8 and so on.

Real output:

[ (187.1.160.84:431) ]

What happens is the packet count keeps going up but the ip just changes to the most recent instead of adding a new one.

Code:

struct node  
{
    char *ip;
    int count;
    struct node *next;
};

struct node *head = NULL;
struct node *current = NULL;

void printInList()
{
    struct node *ptr = head;
    printf("\n[ ");

    //start from the beginning
    while(ptr != NULL)
    {
        printf("(%s:%d) ", ptr->ip, ptr->count);
        ptr = ptr->next;
    }

    printf(" ]");
}


int length()
{
    int length = 0;
    struct node *current;

    for(current = head; current != NULL; current = current->next)
    {
        length++;
    }

    return length;
}
void sort(){

    int i, j, k, tempKey, tempData ;
    struct node *current;
    struct node *next;

    int size = length();
    k = size ;

    for ( i = 0 ; i < size - 1 ; i++, k-- ) {
        current = head ;
        next = head->next ;

        for ( j = 1 ; j < k ; j++ ) {

            if ( current->count < next->count ) {
                tempData = current->count ;
                current->count = next->count;
                next->count = tempData ;

                tempKey = current->ip;
                current->ip = next->ip;
                next->ip = tempKey;
            }

            current = current->next;
            next = next->next;                        
        }
    }
}

void insertInList(char *ipaddr, int count)
{
#ifdef DEBUG
    printf("Inserting: %s:%d\t", ipaddr, count);
#endif
   //create a link
   struct node *link = (struct node*) malloc(sizeof(struct node));

   link->ip = ipaddr;
   link->count = count;
#ifdef DEBUG
    printf("Inserted: %s:%d\t", link->ip, link->count);
#endif
   //point it to old first node
   link->next = head;

   //point first to new first node
   head = link;
}

int checkInList(const char *string)
{
    /* 
    If 1 returned means we found it
    If 0 Means we didnt find it 
    */
    struct node *ptr = head;

    //start from the beginning
    while(ptr != NULL)
    {
#ifdef DEBUG
        printf("Comparing %s and %-20s", ptr->ip, string);
#endif
        if(strcmp(ptr->ip, string) == 0) {
#ifdef DEBUG
            printf("Adding count: %s->%d\n", ptr->ip, ptr->count);
#endif
            ptr->count++;
            return 0;
        }
        ptr = ptr->next;
    }
#ifdef DEBUG
    printf("Returning 1\n");
#endif
    return 1;
}

void *recievethread()
{
    int saddr_size, data_size, sock_raw;
    struct sockaddr_in saddr;
    struct in_addr in;

    unsigned char *buffer = (unsigned char *)malloc(65536);
    sock_raw = socket(AF_INET , SOCK_RAW , IPPROTO_UDP);
    if(sock_raw < 0)
    {
        printf("Socket Error\n");
        exit(1);
    }
    while(1) {
        saddr_size = sizeof saddr;
        data_size = recvfrom(sock_raw , buffer , 65536 , 0 , (struct sockaddr *)&saddr , &saddr_size);
        if(data_size < 0) {
            printf("Recvfrom error , failed to get packets\n");
            exit(1);
        }
        struct iphdr *iph = (struct iphdr*)buffer;
        if(iph->protocol == 17)
        {
            unsigned short iphdrlen = iph->ihl*4;
            struct udphdr *udph = (struct udphdr*)(buffer + iphdrlen);
            unsigned char* payload = buffer + iphdrlen + 8;
            if(ntohs(udph->source) == 123) {
                int body_length = data_size - iphdrlen - 8;
                if (body_length > 47) {
                    if(checkInList(inet_ntoa(saddr.sin_addr)) == 1) {
                        insertInList(inet_ntoa(saddr.sin_addr), 0);
                    }
                }
            }
        }
    }
    close(sock_raw);
}

int main()
{
    pthread_t listenthread;
    pthread_create( &listenthread, NULL, &recievethread, NULL);

    // Some tests
    if(checkInList("192.168.2.1") == 1) {
        insertInList("192.168.2.1", 0);
    }
    if(checkInList("8.8.8.8") == 1) {
        insertInList("8.8.8.8", 0);
    }

    while(1) {
        sort();
        printInList();
        sleep(1);
    }
    printf("\n");
    return 0;
}

Sorry if it doesn't make sense and feel free to redirect me to another thread if you think it will help me.

Answer 1

As others have noted, you need locking around the list to make this correct.

However, the proximate cause of your problem is that inet_ntoa() returns a pointer to a statically-allocated buffer, which is overwritten on the next call. You're recording a pointer to this buffer in your linked list, so the string pointed to by your linked list node changes when you next call inet_ntoa().

You need to duplicate the string when you insert the node:

link->ip = strdup(ipaddr);

---

A simple approach the locking would be to create a global pthread_mutex_t:

pthread_mutex_t list_lock = PTHREAD_MUTEX_INITIALIZER;

then lock it around the list check / insertion:

pthread_mutex_lock(&list_lock);
if (checkInList(inet_ntoa(saddr.sin_addr)) == 1) {
    insertInList(inet_ntoa(saddr.sin_addr), 0);
}
pthread_mutex_unlock(&list_lock);

and around the sort / print:

pthread_mutex_lock(&list_lock);
sort();
printInList();
pthread_mutex_unlock(&list_lock);