packet-parsing #1

Open
pentapiper wants to merge 5 commits from packet-parsing into main
8 changed files with 129 additions and 78 deletions

View file

@ -1,6 +1,7 @@
cmake_minimum_required(VERSION 4.0) cmake_minimum_required(VERSION 4.0)
set(CMAKE_CXX_STANDARD 17) set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_BUILD_TYPE Debug)
project(pupdns VERSION 0.1) project(pupdns VERSION 0.1)
set(CMAKE_EXPORT_COMPILE_COMMANDS ON) set(CMAKE_EXPORT_COMPILE_COMMANDS ON)

View file

@ -1,13 +1,14 @@
#pragma once #pragma once
#include <stddef.h> #include <stddef.h>
#define PACKET_BUFFER_SIZE 512 #define PB_SIZE 512
#define PB_OUT_OF_BOUNDS 9999;
struct DNSPacketBuffer { struct DNSPacketBuffer {

Im used to seeing underscores instead of camelcase in C projects, but thats just an opinion.

Im used to seeing underscores instead of camelcase in C projects, but thats just an opinion.
char buf[PACKET_BUFFER_SIZE]; unsigned char buf[PB_SIZE];
size_t pos; size_t pos;
}; };
struct DNSPacketBuffer *new_dns_packet_buffer(); struct DNSPacketBuffer *new_dns_packet_buffer();

I think both of the structs in this file should be typedef'ed so that you dont need to write 'struct' everywhere.

I think both of the structs in this file should be typedef'ed so that you dont need to write 'struct' everywhere.
struct DNSPacketHeader *dns_header(struct DNSPacketBuffer); unsigned char pb_read_byte(struct DNSPacketBuffer *);

View file

@ -1,6 +1,7 @@
#pragma once #pragma once
#include <stddef.h> #include <stddef.h>
#define DNS_HEADER_BYTES 12
struct DNSPacketHeader { struct DNSPacketHeader {
/* /*
@ -8,14 +9,34 @@ struct DNSPacketHeader {
* a random ID is assigned to query packets. * a random ID is assigned to query packets.
* Response packets must contain the same ID as the query. * Response packets must contain the same ID as the query.
*/ */
size_t packet_id : 16; unsigned int packet_id : 16;
/* /*
* QR (Query/Response) * RD (Recursion Desired)
* 0 if the packet is a query * if set by the client, the server should
* 1 if the packet is a response * attempt to resolve the query recursively
*
* the server may not allow recursive resolution.
* (see RA / Recursion Available)
*/ */
size_t is_response : 1; unsigned int recursion_desired : 1;
/*
* TC (Truncated)
* if true, this message was truncated.
* usually happens when the response payload
* is larger than the max UDP packet size
*
* if set, the query may be re-issued over TCP for the full payload.
*/
unsigned int is_truncated : 1;
/*
* AA (Authoritative Answer)
*
* 1 if the responding server is authoritative for the queried domain
*/
unsigned int is_authoritative : 1;
/* /*
* OPCODE (Operation Code) * OPCODE (Operation Code)
@ -27,51 +48,14 @@ struct DNSPacketHeader {
* *
* almost always will be 0 * almost always will be 0
*/ */
size_t opcode : 4; unsigned int opcode : 4;
/* /*
* AA (Authoritative Answer) * QR (Query/Response)
* * 0 if the packet is a query
* 1 if the responding server is authoritative for the queried domain * 1 if the packet is a response
*/ */
size_t is_authoritative : 1; unsigned int is_response : 1;
/*
* TC (Truncated)
* if true, this message was truncated.
* usually happens when the response payload
* is larger than the max UDP packet size
*
* if set, the query may be re-issued over TCP for the full payload.
*/
size_t is_truncated : 1;
/*
* RD (Recursion Desired)
* if set by the client, the server should
* attempt to resolve the query recursively
*
* the server may not allow recursive resolution.
* (see RA / Recursion Available)
*/
size_t recursion_desired : 1;
/*
* RA (Recursion Available)
* if set by the server, incicates that the server allows
* recursive queries.
*/
size_t recursion_available : 1;
/*
* Z (Reserved)
* in the original spec, this is reserved for future use.
*
* in later RFCs, it is used for DNSSEC queries.
* (to be implemented)
*/
size_t z : 3;
/* /*
* RCODE (Response Code) * RCODE (Response Code)
@ -86,31 +70,47 @@ struct DNSPacketHeader {
* 5 - Refused - the nameserver refuses to perform the requested operation. for example, it may refuse a zone transfer. * 5 - Refused - the nameserver refuses to perform the requested operation. for example, it may refuse a zone transfer.
* 6-15 - Reserved for future use * 6-15 - Reserved for future use
*/ */
size_t response_code : 4; unsigned int response_code : 4;
/*
* Z (Reserved)
* in the original spec, this is reserved for future use.
*
* in later RFCs, it is used for DNSSEC queries.
* (to be implemented)
*/
unsigned int z : 3;
/*
* RA (Recursion Available)
* if set by the server, incicates that the server allows
* recursive queries.
*/
unsigned int recursion_available : 1;
/* /*
* QDCOUNT (Question Count) * QDCOUNT (Question Count)
* the number of entries in the question section * the number of entries in the question section
*/ */
size_t question_count : 16; unsigned int question_count : 16;
/* /*
* ANCOUNT (Answer Count) * ANCOUNT (Answer Count)
* the number of entries in the answer section * the number of entries in the answer section
*/ */
size_t answer_count : 16; unsigned int answer_count : 16;
/* /*
* NSCOUNT (Nameserver Count) * NSCOUNT (Nameserver Count)
* the number of entries in the authority records section * the number of entries in the authority records section
*/ */
size_t ns_count : 16; unsigned int ns_count : 16;
/* /*
* ARCOUNT (Additional Records Count) * ARCOUNT (Additional Records Count)
* the number of entries in the additional records section * the number of entries in the additional records section
*/ */
size_t ar_count : 16; unsigned int ar_count : 16;
}; };
@ -125,26 +125,26 @@ struct RecordPreamble {
* TYPE * TYPE
* the record type * the record type
*/ */
size_t type : 16; unsigned int type : 16;
/* /*
* CLASS * CLASS
* specifies the class of the data in the RDATA field. * specifies the class of the data in the RDATA field.
* in practice, always set to 1 * in practice, always set to 1
*/ */
size_t class : 16; unsigned int class : 16;
/* /*
* TTL (Time-To-Live) * TTL (Time-To-Live)
* how long a record can be cached before it should be requeried. * how long a record can be cached before it should be requeried.
*/ */
size_t ttl : 32; unsigned int ttl : 32;
/* /*
* RDLENGTH (Record Data Length) * RDLENGTH (Record Data Length)
* length of the record-type-specific data. * length of the record-type-specific data.
*/ */
size_t rd_data_length: 16; unsigned int rd_data_length: 16;
}; };
/* /*
@ -153,5 +153,5 @@ struct RecordPreamble {
*/ */
struct ARecord { struct ARecord {
struct RecordPreamble preamble; struct RecordPreamble preamble;
size_t ip : 32; unsigned int ip : 32;
}; };

View file

@ -1,7 +1,10 @@
#pragma once #pragma once
#include "dns_packets.h" #include "dns_packets.h"
#include "dns_packet_buffer.h"
struct DNSRequest { struct DNSRequest {
struct DNSPacketHeader header; struct DNSPacketHeader *header;
}; };
struct DNSRequest *parse_dns_query(struct DNSPacketBuffer *pb);

recommend typedef'ing this so that you can use DNSRequest as a first class type name and not need to add "struct" everywhere.

recommend typedef'ing this so that you can use DNSRequest as a first class type name and not need to add "struct" everywhere.

View file

@ -1,5 +1,5 @@
#pragma once #pragma once
#define UDP_PORT 53 #define UDP_PORT 1053

I think there is a way to leave this at 53 and then use compiler flags to redefine it. Might be a good idea.

Or you could use an IFNDEF clause to have one port for debug builds and one port for release builds based on whether or not cmake is compiling this with debug info.

I think there is a way to leave this at 53 and then use compiler flags to redefine it. Might be a good idea. Or you could use an IFNDEF clause to have one port for debug builds and one port for release builds based on whether or not cmake is compiling this with debug info.
int start_server(); int start_server();

View file

@ -1,4 +1,5 @@
#include "dns_packet_buffer.h" #include "dns_packet_buffer.h"
#include <stdio.h>
#include <stdlib.h> #include <stdlib.h>
struct DNSPacketBuffer *new_dns_packet_buffer() struct DNSPacketBuffer *new_dns_packet_buffer()

What is the point of this getter? I could collapse it down to one line:

struct DNSPacketBuffer *buf = (struct DNSPacketBuffer *) malloc(sizeof(struct DNSPacketBuffer));

If you implement a typedef for this that one liner gets even more concise:

DNSPacketBuffer *buf = (DNSPacketBuffer *) malloc(sizeof(DNSPacketBuffer));

Also, I think it is a really bad idea to have malloc called in scattered functions like this. One honest recommendation I can make is to make sure that the point in code where you need memory should be the same place that you both allocate and free it.

In essence: as much as possible every function that calls malloc should also call free.

I have also seen larger applications keep pools associated with lifetimes. NGINX and NGINX Unit both keep a memory pool to allocate out of attached to a request object. Maybe a simpler implementation could be to create a request_resources struct that tracks shared state through the request processing pipeline. You could store pointers to all your malloc()'ed stuff there and then at the very end implement logic that calls free() on everything attached to the request.

What is the point of this getter? I could collapse it down to one line: ```c struct DNSPacketBuffer *buf = (struct DNSPacketBuffer *) malloc(sizeof(struct DNSPacketBuffer)); ``` If you implement a typedef for this that one liner gets even more concise: ```c DNSPacketBuffer *buf = (DNSPacketBuffer *) malloc(sizeof(DNSPacketBuffer)); ``` Also, I think it is a really bad idea to have malloc called in scattered functions like this. One honest recommendation I can make is to make sure that the point in code where you need memory should be the same place that you both allocate and free it. In essence: as much as possible every function that calls malloc should also call free. I have also seen larger applications keep pools associated with lifetimes. NGINX and NGINX Unit both keep a memory pool to allocate out of attached to a request object. Maybe a simpler implementation could be to create a request_resources struct that tracks shared state through the request processing pipeline. You could store pointers to all your malloc()'ed stuff there and then at the very end implement logic that calls free() on everything attached to the request.
@ -7,8 +8,3 @@ struct DNSPacketBuffer *new_dns_packet_buffer()
buf = malloc(sizeof(struct DNSPacketBuffer)); buf = malloc(sizeof(struct DNSPacketBuffer));
return buf; return buf;
} }
struct DNSPacketHeader *dns_header(struct DNSPacketBuffer)
{
}

View file

@ -1,3 +1,36 @@
#include <stdlib.h>
#include <arpa/inet.h>
#include "dns_request.h" #include "dns_request.h"
struct DNSRequest *parse_dns_query(struct DNSPacketBuffer *pb)
{
struct DNSRequest *req;
req = malloc(sizeof(struct DNSRequest));
req->header = (struct DNSPacketHeader *) pb->buf;
/*
* casting the buffer directly into the packet header struct like this
* works okay, but because of the nature of the DNS protocol, it needs
* a lot of manual tweaking.
*
* the struct fields are defined in a weird order (compared to the spec)
* because C wants to read each byte from right to left,
* but DNS fields go left to right.
*
* so for each set of fields adding up to 8 bits of data, i reversed the
* order of the fields in the struct definition so that the fields end up
* with the correct bits off the wire.
*
* DNS also has multi-bit (but less than a byte) fields such as
* opcode and response code. fortunately we can skip these for now,
* since opcode is practically always zero for our purposes,
* and response_code only makes sense in the server response.
*
* but we do need to take care of all the network-order 16-bit fields.
*/
req->header->packet_id = ntohs(req->header->packet_id);
req->header->question_count = ntohs(req->header->question_count);
return req;
}

View file

@ -2,10 +2,10 @@
#include "dns_packet_buffer.h" #include "dns_packet_buffer.h"
#include "dns_request.h" #include "dns_request.h"
#include <stdlib.h>
#include <stdio.h> #include <stdio.h>
#include <unistd.h> #include <unistd.h>
#include <fcntl.h> #include <fcntl.h>
#include <string.h>
#include <arpa/inet.h> #include <arpa/inet.h>
#include <sys/socket.h> #include <sys/socket.h>
#include <netinet/in.h> #include <netinet/in.h>
@ -37,20 +37,37 @@ int start_server()
return -1; return -1;
} }
socklen_t len = 0; while (true) {
socklen_t len = 0;
int n = recvfrom(sockfd, (char *)pb->buf, sizeof(pb->buf), MSG_WAITALL, 0, &len); int n = recvfrom(sockfd, (char *)pb->buf, sizeof(pb->buf), MSG_WAITALL, 0, &len);
printf("received %d bytes\n", n); printf("===== NEW REQUEST =====\n");
printf("received %d bytes\n", n);
struct DNSRequest *req = new_dns_request(pb); struct DNSRequest *req = parse_dns_query(pb);
printf("packet_id: %x\n", req->header->packet_id); printf("packet_id: %x\n", req->header->packet_id);
printf("is_response: %d\n", req->header->is_response); printf("is_response: %d\n", req->header->is_response);
printf("opcode: %d\n", req->header->opcode); printf("opcode: %d\n", req->header->opcode);
printf("is_authoritative: %d\n", req->header->is_authoritative); printf("is_authoritative: %d\n", req->header->is_authoritative);
printf("is_truncated: %d\n", req->header->is_truncated); printf("is_truncated: %d\n", req->header->is_truncated);
printf("recursion_desired: %d\n", req->header->recursion_desired); printf("recursion_desired: %d\n", req->header->recursion_desired);
printf("recursion_available: %d\n", req->header->recursion_available);
printf("reserved: %d\n", req->header->z);
printf("response_code: %d\n", req->header->response_code);
printf("question_count: %d\n", req->header->question_count);
printf("answer_count: %d\n", req->header->answer_count);
printf("ns_count: %d\n", req->header->ns_count);
printf("ar_count: %d\n", req->header->ar_count);
// for (int i=0; i<n; i++) {
// printf("%d = %02X\n", i, (unsigned char)pb->buf[i]);
// }
free(pb);
free(req);
}
close(sockfd); close(sockfd);
return 0; return 0;