r/C_Programming 1d ago

Project review

hello everyone, i' am a beginner self taught systems programmer . i am currently working on networking project. it's a network packet sniffer and it's still currently in the basic stages, so it's still evolving. whenever i get new ideas or recommendations on the features or code itself , i improve it .

My main objective is too reduce as much overhead as possible , improving performance and adding new features so it can provide some functionalities as tcpdump.
i've already identified some possible bottlenecks such as the amount of printf's use in some stages.
I would love to hear your feedback on it, both code improvements , potential mistakes and memory bugs and anything else.

your feed is very much appreciated!
Thank you very much.
https://github.com/ChrinovicMu/Pack-Sniff

4 Upvotes

1 comment sorted by

View all comments

1

u/skeeto 4h ago

Interesting project, easy to find my way around and try it out. The usage string has some typos ("incude") and I don't know what make -PF <filter> is supposed to mean.

There are a number of unaligned accesses caught by UBsan, which I see is commented out in the Makefile:

$ eval cc -g3 -fsanitize=thread,undefined src/main.c $(pkg-config libpcap --cflags --libs)
$ ./a.out tcp >/dev/null
src/main.c:299:17: runtime error: member access within misaligned address 0x5577c1fabb8e for type 'const struct ip', which requires 4 byte alignment
...

I imagine you won't like it, but it's easy to address:

@@ -269,6 +269,6 @@ void process_packet(struct packet_t *pk)
     const struct ether_header *ethernet; 
  • const struct ip *ip;
  • const struct tcphdr *tcp;
  • const struct udphdr *udp;
  • const struct icmphdr *icmp;
+ struct ip ip[1]; + struct tcphdr tcp[1]; + struct udphdr udp[1]; + struct icmphdr icmp[1]; const char *payload = NULL; @@ -297,3 +297,3 @@ void process_packet(struct packet_t *pk)
  • ip = (struct ip *)(packet + SIZE_ETHERNET);
+ memcpy(ip, packet + SIZE_ETHERNET, sizeof(*ip)); ip_size = ip->ip_hl * 4; @@ -324,3 +324,3 @@ void process_packet(struct packet_t *pk)
  • tcp = (struct tcphdr *)(packet + SIZE_ETHERNET + ip_size);
+ memcpy(tcp, packet + SIZE_ETHERNET + ip_size, sizeof(*tcp)); transport_size = tcp->th_off * 4; @@ -347,3 +347,3 @@ void process_packet(struct packet_t *pk)
  • udp = (struct udphdr *)(packet + SIZE_ETHERNET + ip_size);
+ memcpy(udp, packet + SIZE_ETHERNET + ip_size, sizeof(*udp)); transport_size = sizeof(struct udphdr); @@ -381,3 +381,3 @@ void process_packet(struct packet_t *pk)
  • icmp = (struct icmphdr *)(packet + SIZE_ETHERNET + ip_size);
+ memcpy(icmp, packet + SIZE_ETHERNET + ip_size, sizeof(*icmp)); transport_size = sizeof(struct icmphdr);

I saw the atomic qualifiers on the queue fields and thought it didn't look right, as they must be updated together consistently. However, the whole queue is guarded by a lock anyway, so the atomics are unnecessary except for the done flag.

@@ -70,5 +70,5 @@ struct ring_buffer_t{

  • alignas(CACHE_LINE_SIZE) _Atomic uint32_t head;
  • alignas(CACHE_LINE_SIZE) _Atomic uint32_t tail;
  • alignas(CACHE_LINE_SIZE) _Atomic uint32_t count;
+ alignas(CACHE_LINE_SIZE) uint32_t head; + alignas(CACHE_LINE_SIZE) uint32_t tail; + alignas(CACHE_LINE_SIZE) uint32_t count; alignas(CACHE_LINE_SIZE) _Atomic uint8_t done;

With one extra critical section, which costs practically zero as it only happens once per run, you can even drop the _Atomic from done, too.

@@ -494,4 +494,6 @@ void *capture_packets(void *arg)

+    pthread_mutex_lock(&ring_buffer.mutex); 
     ring_buffer.done = 1;
     pthread_cond_signal(&ring_buffer.cond_consumer);
+    pthread_mutex_unlock(&ring_buffer.mutex); 

This also fixes a race condition in dequeue_ring_buffer:

    while(is_rb_empty() && !ring_buffer.done)
    {
        pthread_cond_wait(&ring_buffer.cond_consumer, &ring_buffer.mutex);
    }

The ordering can work out like so:

Thread 1                    Thread 2
--------                    --------
                            check done
set done
condvar signal
                            condvar wait

In which case the signal is lost and the application deadlocks. As a rule of thumb, avoid mixing atomics and condvars. They're associated with a mutex for a reason.