PPPoE, ARCnet, DECnet and MTP3 assorted improvements #1449
Conversation
|
A few similar fixups are in the works. |
infrastation
force-pushed
the
pppoes_num
branch
from
1f84724 to
b63f023
Compare
infrastation
changed the title
|
This will need an additional type cast to pass Appveyor. Also there is one other incomplete small change in my working copy, if it gets ready soon, it will go into the same pile. @guyharris, the latest commit changes some of the memory management you implemented earlier, please confirm this is as safe as it looks to me. If it is indeed safe for ARCnet addresses, perhaps Ethernet addresses could be simplified the same way (giving a parsing function a pointer to a local variable instead of allocating the memory inside and managing the deallocation outside). |
|
Added a couple more clean-ups in my working copy, now trying to complete the other related improvements, will update when everything seems ready. |
infrastation
force-pushed
the
pppoes_num
branch
from
b63f023 to
1403f34
Compare
|
This now looks about the right number of inter-related commits and is going be merged soon. I understand the memory management change is safe in all contexts. Four commits make changes to the code generator. The filter tests make it easy to see (as far as the test coverage extends) that any changes to the filter programs went exactly as intended, everything else did not change. You are welcome to proof-read. |
infrastation
changed the title
|
The |
|
That's a good question. For the queued changes I have just run a few manual tests for ARCnet, DECnet and MTP3 filtering, everything looks as expected. DECnet has an imperfection that is not specific to this change: Generally speaking, a correctly looking filter program is necessary but not sufficient for correct filtering of packets, so such a class of tests ("this filter expression applied to this sequence of packets produces exactly this sequence of filtering results") is indeed meaningful and it would be useful to do more of them. But how exactly? On the one hand, tcpdump tests could use filter expressions more often, but on the other hand, tcpdump source tree is not the right place for tests that cover libpcap code. The One potential solution could be using a new test program or extending an existing one, for example, Does that make sense? |
infrastation
force-pushed
the
pppoes_num
branch
from
1403f34 to
77cfcb3
Compare
|
Let's see if this revision passes Appveyor. I do not see why the default case in a switch block would not be good enough, but the compiler demanded an explicit value: |
|
Ideally, these new tests should be placed in the libpcap tree, but there is currently no framework for this. In the short term, to validate these changes, some pcap/tests could be added to the tcpdump test directory. |
|
About using a updated filtertest or another program in a second step, I have not a strong opinion at the moment. |
|
Let me see if the |
|
The C leg works: Prototyping the Perl leg... |
|
The Perl leg works. Let me clean this up and add the new tests before these changes to demonstrate the actual filtering continues to work at least for the new test cases. |
Instead of "valid" and "invalid" tests types use "accept" and "reject" because both these types are valid. Spell MTP2 for SIO and MTP3 for DPC, OPC and SLS.
infrastation
force-pushed
the
pppoes_num
branch
from
77cfcb3 to
86dbe27
Compare
|
This revision automatically runs the type of tests I did manually earlier. Passes a complete build matrix with Valgrind on Linux. |
usage() prints the synopsis only, always uses stderr and always exits with an error code. Add the "-h" flag with the standard behaviour of printing a detailed usage message to stdout and exiting normally. Condense the help screen so it fits into an 80-column terminal. Use various exit status codes from sysexits.h to make it easier to differentiate various causes of failure and update TESTrun accordingly. Initialize all variables at declaration time. Use __func__ in an error message.
infrastation
force-pushed
the
pppoes_num
branch
from
86dbe27 to
3648810
Compare
|
This revision includes a workaround for the missing |
In filtertest implement a new "-r <savefile>" mode, which instead of printing the filter program opens the specified savefile, applies the program to each packet and prints the results. In TESTrun implement a new "apply" type of test, which uses the new filtertest mode and compares the printed and the expected results. Declare "apply" test blocks for PPPoE, DECnet and MTP3 and add the associated savefiles. Factor validate_stdout_test() out. Add subroutines to label and to run "apply" tests. Add a single-threaded loop to generate ready-to-run tests. The isup_load_generator.pcap file is from the Wireshark packet capture collection; decnet.pcap, loopback.pcap, pppoe.pcap and pppoes.pcap are from tcpdump tests. All files have been converted to .pcap and made no longer than 10 packets each.
gen_pppoes() compares only the low 16 bits of a [32-bit] word, which does not entirely match the problem space. Eliminate the bitwise AND instruction by comparing a [16-bit] half-word with the same 16 bits, this produces simpler bytecode (thus the corresponding accept test changes) with an equivalent behaviour (thus no apply tests change). While at it, add a comment with the packet header diagram and use a standard named constant for the maximum session ID value.
Both nametoaddr.c and etherent.c are always compiled and each has an identical static copy of xdtoi(). Rename the function to pcapint_xdtoi(), leave only one copy in nametoaddr.c and add a declaration to nametoaddr.h. Rename __pcap_atoin() to pcapint_atoin(), rename __pcap_atodn() to pcapint_atodn(). Add a header guard to nametoaddr.h.
These keywords generate the instructions differently depending on the DLT, so add more tests to cover the Q_LINK case in gen_broadcast() and gen_multicast() completely. Add a few aliases for "link" where it fits.
ARCnet address parsing works incorrectly: $ filtertest ARCNET 'link src $42' (000) ldb [0] (001) jeq #0xd4 jt 2 jf 3 (002) ret #262144 (003) ret #0 Before commit bcbef22 in October 2018 the lexer used pcap_ether_aton() directly and in the case of an AID token pointed the function argument one character past the beginning of the string to skip the initial "$". In the example above the function would parse "42" into the first byte of the MAC address (0x42) and return. Then the lexer passed the token with a partially initialized MAC address to the parser, which passed the MAC address to gen_acode(), which passed it to gen_ahostop(), which correctly used the initialized byte only. Since the commit gen_acode() gets the original string and passes it to pcap_ether_aton(), which in the example above initializes the first MAC address nibble to 0xd, the second to 0x4 and the third (not shown) to 0x2. Refactor this code to fix the bug and to fail more reliably. Implement a new pcapint_atoan() function to parse ARCnet addresses only and to indicate whether the input string is valid. In gen_acode() use the new function instead of pcap_ether_aton() and check the return value before using the result. Ibid., simplify memory management by using a local variable instead of the compiler state for the temporary parsed address. In gen_ahostop() change the address size to 8 bits to have the solution space match the problem space better. Now it works as expected: $ filtertest ARCNET 'link src $42' (000) ldb [0] (001) jeq #0x42 jt 2 jf 3 (002) ret #262144 (003) ret #0 Add accept/apply/reject filter tests to cover the updated implementation and document ARCnet syntax in pcap-filter(7). The bacnet-arcnet-linux.pcap file is from the Wireshark packet capture collection.
gen_dnhostop() either generates a comparison for EtherType or recurses, in which case it either generates... and so on. Although the optimizer deduplicates this later, it is better to avoid the unnecessary work in the first place. Move the EtherType comparison to gen_host(), which is the only caller of gen_dnhostop(). For "decnet host" the resulting unoptimized bytecode is now two instructions shorter (thus the corresponding accept test changes) and identical to the optimized bytecode, the behaviour remains exactly the same (thus no apply tests change).
gen_dnhostop() already compares PLENGTH and FLAGS in parallel, so compare the destination address as well when it is within reach (that is, for the short frame encoding). Fix and expand my earlier comment to document gen_dnhostop() better. This changes the resulting bytecode: it now takes 4 fewer instructions to test a destination DECnet address (thus the corresponding accept tests change), but the behaviour remains the same (thus no apply tests change).
In gen_mtp3field_code_internal() it is not immediately clear what the
OPC value processing code does and whether it does it correctly. I have
verified that it combines byte swapping and bit shifting and indeed
processes the value consistently with the specification. To make it
easier to follow in future, replace the custom code and the custom
bitmask with a combination of SWAPLONG() and a bitwise shift. The
resulting bytecode remains intact (thus no OPC tests change).
The DPC value processing code is plain byte swapping with no bitwise
shift. The comparison uses BPF_W, but DPC spans only two adjacent
bytes, so change it to BPF_H and use SWAPSHORT(), similar to the above.
The resulting bytecode has the same effect (no DPC apply tests change),
but the SNR becomes better, which is the only change in the
corresponding DPC accept tests, e.g.:
-(000) ld [4]
-(001) and #0xff3f0000
-(002) jeq #0xd6310000 jt 3 jf 4
+(000) ldh [4]
+(001) and #0xff3f
+(002) jeq #0xd631 jt 3 jf 4
The SLS value processing code already uses BPF_B, so just reformat it
along the same lines. The resulting bytecode remains intact (thus no
SLS tests change).
All of the above applies the same to the HSL variants ("hdpc", "hopc"
and "hsls"), which differ by the field offsets only. Add comments to
explain the updated code and replace several hard-coded values with
named constants.
infrastation
force-pushed
the
pppoes_num
branch
from
3648810 to
2aebf46
Compare
|
This revision addresses a warning on Windows. |
This addresses some of the semi-related points raised in #1430.