Skip to content

Commit

Permalink
Fix bugs in promote_packet() and demote_packet()
Browse files Browse the repository at this point in the history
* libtrace_pcapfile_pkt_hdr_t, as used by pcapfile, is not the same as
  pcap_pkthdr_t, used by pcap. Notably, libtrace_pcapfile_pkt_hdr_t has
  a 32-bit timestamp and pcap_pkthdr_t uses timeval which varies in size
  depending on the platform.
 - Because of this promote_packet() actually adjusted the timestamp and not the
   caplen or wirelen.
 - Once demoted again the caplen was shorter than it should have been
* Fix demote_packet() to decrease the wire length again
  - This is still missing for non-pcap types
* Fix possible unaligned access in promote_packet (gcc warning)
* Cache calls to trace_get_framing/capture_length, we really shouldn't be
  calling packet operations while modifying the packet, even though the
  values will come from the packet cache.

I think fundamentally promote_packet() should be removed and replaced with
storing the value in the packet's cache. promote_packet() is currently only
used when setting the direction on a pcap: packet but not a pcapfile: packet.
Related to issue #28. Modifying the packet payload when you set a direction
on a packet is unexpected behaviour from a users perspective.

Demote_packet() still seems to be needed for writing packets. But I think we
should copy the packet, so the packet being written isn't modified.
  • Loading branch information
rsanger committed Dec 11, 2020
1 parent bd64180 commit cd8cd74
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 15 deletions.
7 changes: 7 additions & 0 deletions lib/libtrace_int.h
Original file line number Diff line number Diff line change
Expand Up @@ -1321,6 +1321,13 @@ typedef struct libtrace_pcapfile_pkt_hdr_t {
uint32_t wirelen; /* The wire length of the packet */
} libtrace_pcapfile_pkt_hdr_t;

/** Local definition of a PCAP header */
typedef struct libtrace_pcap_pkthdr_t {
struct timeval ts; /* Timestamp */
uint32_t caplen; /* Capture length */
uint32_t wirelen; /* Wire length */
} libtrace_pcap_pkthdr_t;

#ifdef HAVE_DAG
/** Constructor for the DAG format module */
void dag_constructor(void);
Expand Down
40 changes: 25 additions & 15 deletions lib/linktypes.c
Original file line number Diff line number Diff line change
Expand Up @@ -305,31 +305,33 @@ void promote_packet(libtrace_packet_t *packet)
if (packet->trace->format->type == TRACE_FORMAT_PCAP) {
char *tmpbuffer;
libtrace_sll_header_t *hdr;
size_t caplen, framelen;
uint16_t ethertype;

if (pcap_linktype_to_libtrace(rt_to_pcap_linktype(packet->type))
== TRACE_TYPE_LINUX_SLL) {
/* This is already been promoted, so ignore it */
return;
}
caplen = trace_get_capture_length(packet);
framelen = trace_get_framing_length(packet);

/* This should be easy, just prepend the header */
tmpbuffer= (char*)malloc(
sizeof(libtrace_sll_header_t)
+trace_get_capture_length(packet)
+trace_get_framing_length(packet)
);
+ caplen + framelen);

hdr=(libtrace_sll_header_t*)((char*)tmpbuffer
+trace_get_framing_length(packet));
+framelen);

hdr->halen=htons(6);
hdr->pkttype=TRACE_SLL_OUTGOING;

switch(pcap_linktype_to_libtrace(rt_to_pcap_linktype(packet->type))) {
case TRACE_TYPE_NONE:
trace_get_layer3(packet, &hdr->protocol, NULL);
trace_get_layer3(packet, &ethertype, NULL);
hdr->hatype = htons(LIBTRACE_ARPHRD_PPP);
hdr->protocol=htons(hdr->protocol);
hdr->protocol=htons(ethertype);
break;
case TRACE_TYPE_ETH:
hdr->hatype = htons(LIBTRACE_ARPHRD_ETHER);
Expand All @@ -339,11 +341,10 @@ void promote_packet(libtrace_packet_t *packet)
/* failed */
return;
}
memcpy(tmpbuffer,packet->header,
trace_get_framing_length(packet));
memcpy(tmpbuffer, packet->header, framelen);
memcpy(tmpbuffer
+sizeof(libtrace_sll_header_t)
+trace_get_framing_length(packet),
+framelen,
packet->payload,
trace_get_capture_length(packet));
if (packet->buf_control == TRACE_CTRL_EXTERNAL) {
Expand All @@ -354,11 +355,11 @@ void promote_packet(libtrace_packet_t *packet)
}
packet->buffer=tmpbuffer;
packet->header=tmpbuffer;
packet->payload=tmpbuffer+trace_get_framing_length(packet);
packet->payload=tmpbuffer+framelen;
packet->type=pcap_linktype_to_rt(TRACE_DLT_LINUX_SLL);
((struct libtrace_pcapfile_pkt_hdr_t*) packet->header)->caplen+=
((struct libtrace_pcap_pkthdr_t*) packet->header)->caplen +=
sizeof(libtrace_sll_header_t);
((struct libtrace_pcapfile_pkt_hdr_t*) packet->header)->wirelen+=
((struct libtrace_pcap_pkthdr_t*) packet->header)->wirelen +=
sizeof(libtrace_sll_header_t);
trace_clear_cache(packet);
return;
Expand Down Expand Up @@ -461,9 +462,18 @@ bool demote_packet(libtrace_packet_t *packet)
/* Skip the Linux SLL header */
packet->payload=(void*)((char*)packet->payload
+sizeof(libtrace_sll_header_t));
trace_set_capture_length(packet,
trace_get_capture_length(packet)
-sizeof(libtrace_sll_header_t));

if (packet->trace->format->type == TRACE_FORMAT_PCAP) {
((struct libtrace_pcap_pkthdr_t*) packet->header)->caplen -=
sizeof(libtrace_sll_header_t);
((struct libtrace_pcap_pkthdr_t*) packet->header)->wirelen -=
sizeof(libtrace_sll_header_t);
} else {
/* TODO: should we change the wirelen here too? */
trace_set_capture_length(packet,
trace_get_capture_length(packet)
-sizeof(libtrace_sll_header_t));
}

/* Invalidate caches */
trace_clear_cache(packet);
Expand Down

0 comments on commit cd8cd74

Please sign in to comment.