In print_mtrace() the check was just before a GET that fetches the last
field of the structure. The other three checks tested exactly the space
from that GET_IPADDR_STRING() would fetch anyway.
VRRP: Remove a few remaining old truncation props.
Remove the only remaining ND_TCHECK_1() and let the string printing
function guard the snapshot end. While at it, switch the latter to the
one that handles zero padding properly. This makes no difference in the
existing VRRP test, which uses a maximum length password. Lose the
trailing quote so the truncation label justly appears within the
authentication field, not after it.
of_header_print() printed any version as "unknown", always printed the
type in hex only and printed the length with no remarks. If the header
was not good enough to proceed, openflow_print() would just print
"(invalid)" after it and be done with it.
Introduce of10_msgtype_str(). Rename of10_header_body_print() to
of10_message_print() and unburden it of all common header details. Add a
registry of all known OpenFlow version numbers. Merge of_header_print()
into openflow_print() and take the header apart one field at a time with
the captured and the declared lengths in mind, and indicate any anomalies
in version, type and length independently. Try to print the transaction
ID too before giving up on the header.
Among other changes, the header now shows the type as "unknown" when the
decoder does not know which types are valid for the version, and as
"invalid" when the type is definitely not valid for the version. Update
two tests.
Fix a field name for OFPT_QUEUE_GET_CONFIG_REQUEST and
OFPT_QUEUE_GET_CONFIG_REPLY. Update the tests. Use break instead of goto
inside the switch block and lose a label.
Apply this to the 4 the recently converted protocols.
HOW TO USE:
Define this in a print-*.c file before including netdissect.h to make
all its ND_TCHECK*() instances longjmp(). Only do this after verifying
that every ND_TCHECK*() in the file is correct and that in every
possible case it is safe to call longjmp() without incurring resource
leaks or any other problems. If it is not safe or feasible to convert
whole file, consider converting one function at a time.
In openflow_print() terminate the loop if the downstream code managed
to run off the TCP payload end without running off the packet buffer
end (that's how the pointers worked before commit ad69daa2, but I got
the recent conversion to a decrementing unsigned length wrong in commit 4e2e9c24). Expand the comment further.
Declare OF_HEADER_FIXLEN as unsigned to squelch a signedness warning.
Most OpenFlow functions operated on the (cp, ep, len) tuple, returned a
pointer to tell the calling function where to decode next (which was not
a good idea because the calling functions had the values of cp and len in
the first place) and set the pointer to ep to indicate truncated data.
Change them to use the (cp, len) tuple, to be void and to use nd_trunc().
Update the header comment to explain this new convention.
Now if a low-level function gets an offset wrong, as in the bug recently
fixed in of10_queue_props_print(), and manages to return, the error will
not propagate into the calling function, which will be able to continue
the decoding.
Since cp does not have to be current anymore when the [void] function
returns, remove a number of cp increments just before the return.
Introduce OF_FWD(), which modifies both cp and len in sync, and
OF_CHK_FWD(), which also does a bounds check, and use these macros
instead of incrementing cp directly in every function that has both cp
and len arguments. Update the code to account for len that is
decrementing as cp is incrementing.
Commit ad69daa2 was fine for its time, but the introduction of the GET
macros had made it obsolete. Instead of reintroducing ep back merge
of_header_body_print() into openflow_print() to simplify the handling of
pointers, sizes and error conditions, all in the style above.
While at it, subtract OF_HEADER_FIXLEN from the declared OpenFlow message
length before passing it to of10_header_body_print() and make the latter
the only function that has to know about the common header size (clarify
it in the comment). This and the sufficiently current cp and len
disencumber the following functions of type-specific length calculations:
of10_features_reply_print(), of10_flow_mod_print(),
of10_stats_request_print(), of10_stats_reply_print(),
of10_packet_out_print(), of10_packet_in_print() and of10_error_print().
The resulting code in most cases addresses the not yet processed part of
the declared structure with just cp and len: nextfunc(ndo, cp, len) and
ND_TCHECK_LEN(cp, len) do the right thing regardless of how far the
current decoder has made it into the current structure. This makes all
the cp0 and len0 snapshot variables obsolete, so remove them too.
Update a test case output: the missing closing quote now correctly means
that the snapshot end is inside the "data" field of a BSN_SHELL_COMMAND
vendor message, not after it.
In of10_data_print() make an existing pre-ndo_vflag ND_TCHECK_LEN() call
conditional to let hex_and_ascii_print() print the data that is within
the packet buffer.
In of10_packet_data_print() make an existing post-ndo_vflag
ND_TCHECK_LEN() call conditional to make bounds checks complete for the
current message if it is an early return and to let ether_print() print
the data that is within the packet buffer if it is not.
In of10_match_print() make an existing ND_TCHECK_2() call conditional
with a comment.
In of10_actions_print() add a conditional ND_TCHECK_2() call to have the
final OFPAT_OUTPUT action bounds fully checked even if it is not fully
decoded.
Lose a few ND_TCHECK calls on padding that is followed by an eventual
unconditional bounds check within the same structure/iteration.
Add a number of comments to tell that a bounds check (or an absence
thereof) is intentional.
Lose unused items, lose _HEADER_ in all names except the common OpenFlow
header, always spell _REQUEST_ and _REPLY_ where they belong, spell
either _MINLEN or _FIXLEN everywhere. Denote the 2 mock octets once
again.
In of10_queue_props_print() the "skip" branch tested and skipped 4 bytes
too many, so a malformed queue property would likely result in an odd
truncation or incorrect decoding of subsequent data (this is based on
code analysis only).
Get the offset and the size right and add a comment to explain it. Add
and update other comments to clarify a few similar subtleties that are
easy to get wrong.
"(int_part != 0) && (fraction != 0)" matches a value that has both
seconds and seconds fraction set to a non-zero value, hence it leaves
zero seconds (0.123) and exact seconds (123.0) out. This bug originates
in commit d1b0faed from 2004.
Fix the expression and update a test that, although prints its only
Receiver Timestamp as 0.000000000, in fact has seconds set to 0 and
seconds fraction set to 2.
ND_TCHECK_LEN() just before an unconditional GET_ETHERADDR_STRING() is
excess. ND_TCHECK_LEN() just before a conditional GET_ETHERADDR_STRING()
can be removed where there is a subsequent unconditional GET_ call (this
is not specific to GET_ETHERADDR_STRING(), for example, unconditional
fetch of "dl_type" makes sure that it is safe to have the fetch of
"dl_vlan_pcp" conditional).
Similarly to earlier commits, follow up from commit 19b51ded and make
changes along the same lines. Also make all functions void, lose a few
excess ND_TCHECK_LEN() instances, update comments, reduce scope for a
couple variables, rearrange some line breaks and call ND_TCHECK_LEN()
for unknown protocol versions.
Similarly to the previous commit, follow up from commit 5d938abe and
simplify the pointer and size arithmetics. Lose a ND_TCHECK_LEN() before
nd_printn() so the latter can print as much of the counted string as
possible. Use nd_trunc().
The change made in commit bdd97f54 was safe, but was slightly incorrect.
Before the change the decoder used the _declared_ end of packet (the
initial "cp" + the initial "len") as the target of how much data to try
to decode, and whilst trying to do that it could cross the _captured_ end
of packet (ndo_snapend) and give up complaining, so the output would
correctly indicate there was supposed to be more data beyond the
unexpected snapshot end.
After the change the decoder started to use the captured end of packet to
tell where the packet ends, so it would never cross ndo_snapend, but it
would interpret the packet data as if the missing part was never supposed
to be there and would thus decode truncated packets incorrectly.
In order to eliminate this space for misinterpretation, remove the "ep"
pointer and change the code both to advance "cp" and to decrease "len" as
it is going through the protocol structures, so the part of the packet
that is supposed to be present but has not been decoded yet can be
addressed by just "cp" and "len" (like it is done in many functions
elsewhere). Let the GET_ macros guard the snapshot end and use nd_trunc()
to handle the remaining ND_TCHECK_LEN() instances.
Articulate the part of the message sized by skipCount.
This new function is a part of item 2 of the longjmp() conversion plan
Francois-Xavier proposed on tcpdump-workers. As a noreturn function, it
can fit uniformly at the end of any decoder function, regardless if void
or not, for example:
const u_char *
something_print (netdissect_options *ndo, const u_char *cp, u_int len)
{
/* (some normal decoding) */
ND_TCHECK_2(cp);
/* (some more normal decoding) */
return cp + len;
trunc:
nd_trunc(ndo);
/* The compiler does not warn about a missing return. */
}
...the only effect of the ND_TCHECK_LEN() call is that when the protocol
field is not entirely within the packet buffer, the code before
triggering a longjmp() prints nothing instead of printing the part of
the field that is within the packet buffer. Thus remove these instances
of ND_TCHECK_LEN(), so the output better represents the captured data.
As struct pkt_id informally notes it, the site name string follows
"seqptr's", which is a sequence of zero or more (as encoded in the "nid"
field) struct id_off items. Try to print the string after trying to
print the sequence, so the output makes as much sense as possible for
truncated packets. Also remove one ND_TTEST_LEN() before a nd_print() so
the latter prints as much data as is available and deals with the
snapshot end.
While at it, rename and retype a pointer for clarity and lose two excess
type casts, also add a length sanity check.
In this file an ASCII tab character means "one level of indentation,
whatever amount of whitespace an editor uses to _display_ it", and a
sequence of several spaces means "exactly this many spaces". Sometimes
they are mixed together for a specific purpose, which is easy to see in
conditional complex print statements: the ND_PRINT() line is one more
level of indentation deeper than the if(), and the wrapped remainder in
addition to that is as many spaces deeper as necessary to align with the
first argument. It is not the simplest thing to explain, but easy to see
how it works in the actual code.
Wrap and unwrap some lines to accommodate the code churn, assuming that
one level of indentation is worth at most 8 positions and that most lines
should look at most 80 characters long unless there is a good reason to
keep them longer.
Make version and usage printing more consistent. [skip ci]
When there is an error, print to stderr and exit with a non-0 status.
Otherwise print to stdout and exit with a status 0. See also tcpslice
commit 5015245.
As discussed on GitHub and on the mailing list, install tcpdump into
bindir because it can be useful to non-root users too, in ways that do
not involve doing live packet captures. Original idea by Guy Harris.
Remove ND_TCHECK_SIZE() immediately before two GET_BE_U_2() calls that
together fully cover a struct forces_tlv.
Ditto before two GET_BE_U_4() calls and a struct forces_ilv.
Remove ND_TCHECK_SIZE() immediately before ilv_valid(), which always
checks that forces_ilv->length is within bounds, which implies that
forces_ilv->type is also within bounds, which means the whole 8-byte
structure is within bounds.
Ditto before GET_BE_U_2() on the length field of a struct forces_tlv.
Remove a number of instances that do not match common patterns and have
the only substantial effect on the code flow that a truncated packet
triggers "goto trunc" instead of longjmp(). (In a few cases this change
can increase the number of fields printed before giving up.)
Use FreeBSD 11.4-RELEASE and 12.1-RELEASE instead of 11.3-CURRENT and
12.1-CURRENT, this allows to drop "pkg update -f", which was a
workaround for 11.3-CURRENT (see commit 96c4db3), and which had later
broken in 13.0-CURRENT. Relates to cirruslabs/cirrus-ci-docs#695.
This first revision is loosely based on the output of
git log | grep -E '^Author: ' | sort -u
and some heuristics. It does not cover every case, but it results in
more consistent output from "git blame" and "git log --use-mailmap".
Denis Ovsienko [Sat, 29 Aug 2020 01:48:07 +0000 (02:48 +0100)]
Report periodic stats only when safe to do so. [skip ci]
As explained in GH #155, when tcpdump is given -r, -w and -v and it
takes long enough to read from the input file (because it is stdin
connected through network or a pipe to stdout of another tcpdump doing
a live capture), pcap_loop() will error before long. One of the ways to
reproduce the fault is as follows:
Denis Ovsienko [Thu, 27 Aug 2020 22:04:49 +0000 (23:04 +0100)]
RADIUS: Redo the RFC 5580 test for Solaris. [skip ci]
The file tests/RADIUS-RFC5580.pcap had all NTP timestamps set to
0x01234567.89abcdef seconds since year 1900, which on Linux and FreeBSD
produced "19088743.537777777 (1900-08-09T22:25:43Z)", but on Solaris 10
produced "(unrepresentable)". Increment Sighting Time by 120 nominal
years worth of seconds and make Time-to-Live and Retention Expires 12
and 48 hours later respectively.
Denis Ovsienko [Thu, 27 Aug 2020 12:48:08 +0000 (13:48 +0100)]
RADIUS: Fixup the previous commit.
Use GET_ macros and C99 uint types. Use nd_printn() to print RADIUS
strings, which are not NUL-terminated (see RFC 2865 page 25). Use
p_ntp_time() to print NTP timestamps. In print_attr_location_data() print
the Location field as hex and add a comment to explain why. In
print_basic_location_policy_rules() display any non-zero MBZ bits in the
Flags field. Make output format more consistent. Update the test case.
Denis Ovsienko [Wed, 12 Aug 2020 23:59:44 +0000 (00:59 +0100)]
Rx: Make UDP ports 16-bit to compile (GH #868).
Gisle Vanem reported that GCC 7.1 for DJGPP sees u_int and uint32_t as
two different types, hence the forward declaration for rx_cache_find()
was different from the actual function. UDP port numbers are 16-bit and
udp_print() appropriately uses uint16_t for them. Use the same type in
the downstream Rx-specific code to make things simpler and consistent.
projects / tcpdump / log
