Removing remaining comments. Looks fine.

This commit is contained in:
Robin Sommer 2012-03-23 16:40:14 -07:00
parent c765f43fe3
commit 4321f635ac
10 changed files with 9 additions and 75 deletions

@ -1 +1 @@
Subproject commit 3034da8f082b61157e234237993ffd7a95be6e62
Subproject commit dd1a3a95f07082efcd5274b21104a038d523d132

@ -1 +1 @@
Subproject commit f53bcb2b492cb0db3dd288384040abc2ab711767
Subproject commit a59b35bdada8f70fb1a59bf7bb2976534c86d378

@ -1 +1 @@
Subproject commit a08ca90727c5c4b90aa8633106ec33a5cf7378d4
Subproject commit 612e95ac62a06b32b2e9e627f30527012a89a12c

@ -1 +1 @@
Subproject commit 954538514d71983e7ef3f0e109960466096e1c1d
Subproject commit 66e9e87beebce983fa0f479b0284d5690b0290d4

2
cmake

@ -1 +1 @@
Subproject commit 2cc105577044a2d214124568f3f2496ed2ccbb34
Subproject commit 550ab2c8d95b1d3e18e40a903152650e6c7a3c45

View file

@ -933,11 +933,7 @@ const ICMP_UNREACH_ADMIN_PROHIB = 13; ##< Adminstratively prohibited.
# Definitions for access to packet headers. Currently only used for
# discarders.
# todo::these should go into an enum to make them autodoc'able
const IPPROTO_IP = 0; ##< Dummy for IP. [Robin] Rename to IPPROTO_IP4?
# [Jon] I'd say leave it be or remove it because from <netinet/in.h>
# IPPROTO_IPV4 can actually be the same as IPPROTO_IPIP (4)...
# IPPROTO_IP seems to be just for use with the socket API and not
# actually identifying protocol numbers in packet headers
const IPPROTO_IP = 0; ##< Dummy for IP.
const IPPROTO_ICMP = 1; ##< Control message protocol.
const IPPROTO_IGMP = 2; ##< Group management protocol.
const IPPROTO_IPIP = 4; ##< IP encapsulation in IP.
@ -947,14 +943,6 @@ const IPPROTO_IPV6 = 41; ##< IPv6 header.
const IPPROTO_RAW = 255; ##< Raw IP packet.
# Definitions for IPv6 extension headers.
# [Robin] Do we need a constant for unknown extensions?
# [Jon] I don't think so, these constants are just conveniences to improve
# script readability, but they also identify the actual assigned protocol
# number of the header type. If the core were to actually pass to the
# script-layer a next-header value of something we don't know about yet,
# that value would be the actual value seen in the packet, not something
# we should make up. We could provide a "KNOWN_PROTOCOLS" set for
# convenience that one could check membership against.
const IPPROTO_HOPOPTS = 0; ##< IPv6 hop-by-hop-options header.
const IPPROTO_ROUTING = 43; ##< IPv6 routing header.
const IPPROTO_FRAGMENT = 44; ##< IPv6 fragment header.
@ -1068,15 +1056,6 @@ type ip6_esp: record {
##
## .. bro:see:: pkt_hdr ip4_hdr ip6_hopopts ip6_dstopts ip6_routing ip6_fragment
## ip6_ah ip6_esp
#
# [Robin] What happens to unknown extension headers? We should keep them too so that
# one can at least identify what one can't analyze.
# [Jon] Currently, they show up as "unknown_protocol" weirds and those packets
# are skipped before any "new_packet" or "ipv6_ext_headers" events are
# raised as those depend on a connection parameter which can't be
# created since we can't parse past unknown extension headers to get
# at the upper layer protocol. Does that seem reasonable for at
# being able to identify things that couldn't be analyzed?
type ip6_ext_hdr: record {
## The RFC 1700 et seq. IANA assigned number identifying the type of
## the extension header.
@ -1170,11 +1149,6 @@ type icmp_hdr: record {
## A packet header, consisting of an IP header and transport-layer header.
##
## .. bro:see:: new_packet
#
# [Robin] Add flags saying whether it's v4/v6, tcp/udp/icmp? The day will come where
# we can't infer that from the connection anymore (tunnels).
# [Jon] I'm not sure what you mean, doesn't checking result of ?$ operator
# always work for finding out protocols involved?
type pkt_hdr: record {
ip: ip4_hdr &optional; ##< The IPv4 header if an IPv4 packet.
ip6: ip6_hdr &optional; ##< The IPv6 header if an IPv6 packet.

View file

@ -33,10 +33,6 @@ FragReassembler::FragReassembler(NetSessions* arg_s,
s = arg_s;
key = k;
// [Robin] Can't we merge these two cases now?
// [Jon] I think we'll always have to check v4 versus v6 to get the correct
// proto_hdr_len unless IP_Hdr::HdrLen itself makes a special case for
// IPv6 fragments (but that seems more confusing to me)
const struct ip* ip4 = ip->IP4_Hdr();
if ( ip4 )
{

View file

@ -14,22 +14,6 @@
#include <netinet/ip.h>
#include <netinet/ip6.h>
// [Robin] I'm concerced about the virtual methods here. These methods will
// be called *a lot* and that may add to some significant overhead I'm afraid
// (at least eventually as IPv6 is picking up).
//
// [Robin] Similar concern for the vector<IPv6_Hdr*> and ip6_hdrs data
// members: we're creating/allocating those for every IPv6 packet, right?
//
// Any idea how to avoid these?
//
// [Jon] Seems fair enough to just remove the virtual method concern at this
// point by replacing the class hierarchy with some inline functions that
// do switch statements. I don't know what to do about the
// vector<IPv6_hdr*> and ip6_hdrs data members being allocated for every
// IPv6 packet, maybe it's too early to try to optimize before we know
// the frequency at which extension headers appear in real IPv6 traffic?
/**
* Base class for IPv6 header/extensions.
*/

View file

@ -42,7 +42,6 @@ extern int select(int, fd_set *, fd_set *, fd_set *, struct timeval *);
PList(PktSrc) pkt_srcs;
// FIXME: We should really merge PktDumper and PacketDumper.
// It's on my to-do [Robin].
PktDumper* pkt_dumper = 0;
int reading_live = 0;

View file

@ -430,13 +430,6 @@ void NetSessions::DoNextPacket(double t, const struct pcap_pkthdr* hdr,
if ( discarder && discarder->NextPacket(ip_hdr, len, caplen) )
return;
// [Robin] dump_this_packet = 1 for non-ICMP/UDP/TCP removed here. Why?
// [Jon] The default case of the "switch ( proto )" calls Weird() which
// should set dump_this_packet = 1. The old code also returned
// at this point for non-ICMP/UDP/TCP, but for IPv6 fragments
// we need to do the reassembly first before knowing for sure what
// upper-layer protocol it is.
FragReassembler* f = 0;
if ( ip_hdr->IsFragment() )
@ -472,10 +465,8 @@ void NetSessions::DoNextPacket(double t, const struct pcap_pkthdr* hdr,
len -= ip_hdr_len; // remove IP header
caplen -= ip_hdr_len;
// [Robin] Does ESP need to be the last header?
// [Jon] In terms of what we try to parse, yes, we can't go any further
// in parsing a header chain once we reach an ESP one since
// encrypted payload immediately follows.
// We stop building the chain when seeing IPPROTO_ESP so if it's
// there, it's always the last.
if ( ip_hdr->LastHeader() == IPPROTO_ESP )
{
dump_this_packet = 1;
@ -498,16 +489,6 @@ void NetSessions::DoNextPacket(double t, const struct pcap_pkthdr* hdr,
return;
}
// [Robin] The Remove(f) used to be here, while it's now before every
// return statement. I'm not seeing why?
// [Jon] That Remove(f) is still here above in the CheckHeaderTrunc()
// conditional that's just a refactoring of the old code.
// The reason why it's not done unconditionally after the reassembly
// is because doing that could cause the object that ip_hdr points
// to to be freed when we still need to use that below.
// I added Remove(f)'s before other "abnormal" return points that
// looked like they'd otherwise leak the memory.
const u_char* data = ip_hdr->Payload();
ConnID id;
@ -553,7 +534,7 @@ void NetSessions::DoNextPacket(double t, const struct pcap_pkthdr* hdr,
}
default:
Weird(fmt("unknown_protocol %d", proto), hdr, pkt);
Weird(fmt("unknown_protocol_%d", proto), hdr, pkt);
Remove(f);
return;
}