Fix, test and document ARCnet address parsing.

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.

diff --git a/gencode.c b/gencode.c
index dd0189b8b0f6b5f0273ee0b6ceda2c773717108a..e6bc7b891c6ac961186ab337e0b41654811db88b 100644 (file)
--- a/gencode.c
+++ b/gencode.c
@@ -663,7 +663,7 @@ static struct block *gen_hostop(compiler_state_t *, bpf_u_int32, bpf_u_int32,
 static struct block *gen_hostop6(compiler_state_t *, struct in6_addr *,
     struct in6_addr *, int, bpf_u_int32, u_int, u_int);
 #endif
 static struct block *gen_hostop6(compiler_state_t *, struct in6_addr *,
     struct in6_addr *, int, bpf_u_int32, u_int, u_int);
 #endif

-static struct block *gen_ahostop(compiler_state_t *, const u_char *, int);
+static struct block *gen_ahostop(compiler_state_t *, const uint8_t, int);

 static struct block *gen_ehostop(compiler_state_t *, const u_char *, int);
 static struct block *gen_fhostop(compiler_state_t *, const u_char *, int);
 static struct block *gen_thostop(compiler_state_t *, const u_char *, int);
 static struct block *gen_ehostop(compiler_state_t *, const u_char *, int);
 static struct block *gen_fhostop(compiler_state_t *, const u_char *, int);
 static struct block *gen_thostop(compiler_state_t *, const u_char *, int);


@@ -8390,8 +8390,6 @@ gen_byteop(compiler_state_t *cstate, int op, int idx, bpf_u_int32 val)
        return b;
 }
 
        return b;
 }
 

-static const u_char abroadcast[] = { 0x0 };
-

 struct block *
 gen_broadcast(compiler_state_t *cstate, int proto)
 {
 struct block *
 gen_broadcast(compiler_state_t *cstate, int proto)
 {


@@ -8413,7 +8411,8 @@ gen_broadcast(compiler_state_t *cstate, int proto)
                switch (cstate->linktype) {
                case DLT_ARCNET:
                case DLT_ARCNET_LINUX:
                switch (cstate->linktype) {
                case DLT_ARCNET:
                case DLT_ARCNET_LINUX:

-                       return gen_ahostop(cstate, abroadcast, Q_DST);
+                       // ARCnet broadcast is [8-bit] destination address 0.
+                       return gen_ahostop(cstate, 0, Q_DST);

                case DLT_EN10MB:
                case DLT_NETANALYZER:
                case DLT_NETANALYZER_TRANSPARENT:
                case DLT_EN10MB:
                case DLT_NETANALYZER:
                case DLT_NETANALYZER_TRANSPARENT:


@@ -8498,8 +8497,8 @@ gen_multicast(compiler_state_t *cstate, int proto)
                switch (cstate->linktype) {
                case DLT_ARCNET:
                case DLT_ARCNET_LINUX:
                switch (cstate->linktype) {
                case DLT_ARCNET:
                case DLT_ARCNET_LINUX:

-                       /* all ARCnet multicasts use the same address */
-                       return gen_ahostop(cstate, abroadcast, Q_DST);
+                       // ARCnet multicast is the same as broadcast.
+                       return gen_ahostop(cstate, 0, Q_DST);

                case DLT_EN10MB:
                case DLT_NETANALYZER:
                case DLT_NETANALYZER_TRANSPARENT:
                case DLT_EN10MB:
                case DLT_NETANALYZER:
                case DLT_NETANALYZER_TRANSPARENT:


@@ -9053,11 +9052,10 @@ gen_p80211_fcdir(compiler_state_t *cstate, bpf_u_int32 fcdir)
        return (b0);
 }
 
        return (b0);
 }
 

+// Process an ARCnet host address string.

 struct block *
 gen_acode(compiler_state_t *cstate, const char *s, struct qual q)
 {
 struct block *
 gen_acode(compiler_state_t *cstate, const char *s, struct qual q)
 {

-       struct block *b;
-

        /*
         * Catch errors reported by us and routines below us, and return NULL
         * on an error.
        /*
         * Catch errors reported by us and routines below us, and return NULL
         * on an error.


@@ -9071,13 +9069,16 @@ gen_acode(compiler_state_t *cstate, const char *s, struct qual q)
        case DLT_ARCNET_LINUX:
                if ((q.addr == Q_HOST || q.addr == Q_DEFAULT) &&
                    q.proto == Q_LINK) {
        case DLT_ARCNET_LINUX:
                if ((q.addr == Q_HOST || q.addr == Q_DEFAULT) &&
                    q.proto == Q_LINK) {

-                       cstate->e = pcap_ether_aton(s);
-                       if (cstate->e == NULL)
-                               bpf_error(cstate, "malloc");
-                       b = gen_ahostop(cstate, cstate->e, (int)q.dir);
-                       free(cstate->e);
-                       cstate->e = NULL;
-                       return (b);
+                       uint8_t addr;
+                       /*
+                        * The lexer currently defines the address format in a
+                        * way that makes this error condition never true.
+                        * Let's check it anyway in case this part of the lexer
+                        * changes in future.
+                        */
+                       if (! pcapint_atoan(s, &addr))
+                           bpf_error(cstate, "invalid ARCnet address '%s'", s);
+                       return gen_ahostop(cstate, addr, (int)q.dir);

                } else
                        bpf_error(cstate, "ARCnet address used in non-arc expression");
                /*NOTREACHED*/
                } else
                        bpf_error(cstate, "ARCnet address used in non-arc expression");
                /*NOTREACHED*/


@@ -9088,18 +9089,25 @@ gen_acode(compiler_state_t *cstate, const char *s, struct qual q)
        }
 }
 
        }
 }
 

+// Compare an ARCnet host address with the given value.

 static struct block *
 static struct block *

-gen_ahostop(compiler_state_t *cstate, const u_char *eaddr, int dir)
+gen_ahostop(compiler_state_t *cstate, const uint8_t eaddr, int dir)

 {
        register struct block *b0, *b1;
 
        switch (dir) {
 {
        register struct block *b0, *b1;
 
        switch (dir) {

-       /* src comes first, different from Ethernet */
+       /*
+        * ARCnet is different from Ethernet: the source address comes before
+        * the destination address, each is one byte long.  This holds for all
+        * three "buffer formats" in RFC 1201 Section 2.1, see also page 4-10
+        * in the 1983 edition of the "ARCNET Designer's Handbook" published
+        * by Datapoint (document number 61610-01).
+        */

        case Q_SRC:
        case Q_SRC:

-               return gen_bcmp(cstate, OR_LINKHDR, 0, 1, eaddr);
+               return gen_cmp(cstate, OR_LINKHDR, 0, BPF_B, eaddr);

 
        case Q_DST:
 
        case Q_DST:

-               return gen_bcmp(cstate, OR_LINKHDR, 1, 1, eaddr);
+               return gen_cmp(cstate, OR_LINKHDR, 1, BPF_B, eaddr);

 
        case Q_AND:
                b0 = gen_ahostop(cstate, eaddr, Q_SRC);
 
        case Q_AND:
                b0 = gen_ahostop(cstate, eaddr, Q_SRC);

diff --git a/nametoaddr.c b/nametoaddr.c
index ee74ffdfb3d6aef22e8e86c580e1a29f5993209c..512ed4dbfd57f3ce80d82ab838453e48fb88e375 100644 (file)
--- a/nametoaddr.c
+++ b/nametoaddr.c
@@ -728,6 +728,64 @@ pcapint_atodn(const char *s, bpf_u_int32 *addr)
        return(32);
 }
 
        return(32);
 }
 

+/*
+ * libpcap ARCnet address format is "^\$[0-9a-fA-F]{1,2}$" in regexp syntax.
+ * Iff the given string is a well-formed ARCnet address, parse the string,
+ * store the 8-bit unsigned value into the provided integer and return 1.
+ * Otherwise return 0.
+ *
+ *  --> START -- $ --> DOLLAR -- [0-9a-fA-F] --> HEX1 -- \0 -->-+
+ *        |              |                        |             |
+ *       [.]            [.]                  [0-9a-fA-F]        |
+ *        |              |                        |             |
+ *        v              v                        v             v
+ *    (invalid) <--------+-<---------------[.]-- HEX2 -- \0 -->-+--> (valid)
+ */
+int
+pcapint_atoan(const char *s, uint8_t *addr)
+{
+       enum {
+               START,
+               DOLLAR,
+               HEX1,
+               HEX2,
+       } fsm_state = START;
+       uint8_t tmp = 0;
+
+       while (*s) {
+               switch (fsm_state) {
+               case START:
+                       if (*s != '$')
+                               goto invalid;
+                       fsm_state = DOLLAR;
+                       break;
+               case DOLLAR:
+                       if (! PCAP_ISXDIGIT(*s))
+                               goto invalid;
+                       tmp = pcapint_xdtoi(*s);
+                       fsm_state = HEX1;
+                       break;
+               case HEX1:
+                       if (! PCAP_ISXDIGIT(*s))
+                               goto invalid;
+                       tmp <<= 4;
+                       tmp |= pcapint_xdtoi(*s);
+                       fsm_state = HEX2;
+                       break;
+               case HEX2:
+                       goto invalid;
+               } // switch
+               s++;
+       } // while
+       if (fsm_state == HEX1 || fsm_state == HEX2) {
+               *addr = tmp;
+               return 1;
+       }
+
+invalid:
+       return 0;
+}
+

 /*
  * Convert 's', which can have the one of the forms:
  *
 /*
  * Convert 's', which can have the one of the forms:
  *

diff --git a/nametoaddr.h b/nametoaddr.h
index 863918fad2fd0e30919453615a989ad84948136f..02ac9e1da4ebdb4bf7080e3d4b3721b748917e41 100644 (file)
--- a/nametoaddr.h
+++ b/nametoaddr.h
@@ -44,6 +44,7 @@ extern "C" {
  */
 extern int pcapint_atodn(const char *, bpf_u_int32 *);
 extern int pcapint_atoin(const char *, bpf_u_int32 *);
  */
 extern int pcapint_atodn(const char *, bpf_u_int32 *);
 extern int pcapint_atoin(const char *, bpf_u_int32 *);

+extern int pcapint_atoan(const char *, uint8_t *);

 extern u_char pcapint_xdtoi(const u_char);
 
 #ifdef __cplusplus
 extern u_char pcapint_xdtoi(const u_char);
 
 #ifdef __cplusplus

diff --git a/pcap-filter.manmisc.in b/pcap-filter.manmisc.in
index 6ffbe0c66426d9510ab10c163bfcb880d2f8acc6..cfad7b102c04e379a070f638b66217650d71c330 100644 (file)
--- a/pcap-filter.manmisc.in
+++ b/pcap-filter.manmisc.in
@@ -18,7 +18,7 @@
 .\" WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED WARRANTIES OF
 .\" MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE.
 .\"
 .\" WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED WARRANTIES OF
 .\" MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE.
 .\"

-.TH PCAP-FILTER @MAN_MISC_INFO@ "28 January 2025"
+.TH PCAP-FILTER @MAN_MISC_INFO@ "29 January 2025"

 .SH NAME
 pcap-filter \- packet filter syntax
 .br
 .SH NAME
 pcap-filter \- packet filter syntax
 .br


@@ -1032,6 +1032,38 @@ and
 respectively, but only if the MTP2 link uses the extended sequence numbers
 encoding specified for high speed signalling links (HSL) in ITU-T
 Recommendation Q.703 Annex A.
 respectively, but only if the MTP2 link uses the extended sequence numbers
 encoding specified for high speed signalling links (HSL) in ITU-T
 Recommendation Q.703 Annex A.

+.IP "\fBlink src \fIarcnetaddr\fR"
+True, only for
+.B \%DLT_ARCNET
+or
+.BR \%DLT_ARCNET_LINUX ,
+if the ARCnet source address is
+.IR \%arcnetaddr ,
+which in this expression is a string of the form
+.B $xx
+or
+.BR $x ,
+where "x" is a hexadecimal digit (for example,
+.BR $2b ).
+Note that this address syntax clashes with the parameter expansion syntax
+in POSIX-compatible shells and elsewhere, so depending on the use case the
+filter string may require the use of single quotes or a backslash.
+.IP "\fBlink dst \fIarcnetaddr\fR"
+Same as the above, but for the destination ARCnet address; also in ARCnet
+context
+.B \%broadcast
+and
+.B \%multicast
+are equivalent to
+.BR "\%link dst $0" .
+.IP "\fBlink host \fIarcnetaddr\fR"
+Abbreviation for:
+.in +.5i
+.nf
+\fBlink src \fIarcnetaddr \fBor link dst \fIarcnetaddr\fR
+.fi
+.in -.5i
+in the expressions above.

 .IP  "\fIexpr1 relop expr2\fR"
 True if the relation holds.  \fIRelop\fR is one of
 .RB { > ,
 .IP  "\fIexpr1 relop expr2\fR"
 True if the relation holds.  \fIRelop\fR is one of
 .RB { > ,

diff --git a/testprogs/TESTrun b/testprogs/TESTrun
index 304445640d2f02c49d20cbf348c90d7e1b846d60..8b9a2e0154f27b0eb0b6710874281246c22d6285 100755 (executable)
--- a/testprogs/TESTrun
+++ b/testprogs/TESTrun
@@ -1593,6 +1593,7 @@ my %accept_blocks = (
                        'multicast',
                        'link broadcast',
                        'link multicast',
                        'multicast',
                        'link broadcast',
                        'link multicast',

+                       'link dst $00',

                ],
                unopt => <<~'EOF',
                        (000) ldb      [1]
                ],
                unopt => <<~'EOF',
                        (000) ldb      [1]


@@ -1601,6 +1602,47 @@ my %accept_blocks = (
                        (003) ret      #0
                        EOF
        }, # arcnet_broadcast_multicast
                        (003) ret      #0
                        EOF
        }, # arcnet_broadcast_multicast

+       arcnet_host => {
+               DLT => 'ARCNET',
+               expr => 'link host $0e',
+               aliases => [
+                       'link src or dst host $0e',
+                       'link src or dst $0e',
+                       'link host $e',
+                       'link src or dst host $e',
+                       'link src or dst $e',
+               ],
+               unopt => <<~'EOF',
+                       (000) ldb      [0]
+                       (001) jeq      #0xe             jt 4    jf 2
+                       (002) ldb      [1]
+                       (003) jeq      #0xe             jt 4    jf 5
+                       (004) ret      #262144
+                       (005) ret      #0
+                       EOF
+       }, # arcnet_host
+       arcnet_src_host => {
+               DLT => 'ARCNET',
+               expr => 'link src host $8c',
+               aliases => ['link src $8c'],
+               unopt => <<~'EOF',
+                       (000) ldb      [0]
+                       (001) jeq      #0x8c            jt 2    jf 3
+                       (002) ret      #262144
+                       (003) ret      #0
+                       EOF
+       }, # arcnet_src_host
+       arcnet_dst_host => {
+               DLT => 'ARCNET',
+               expr => 'link dst host $a4',
+               aliases => ['link dst $a4'],
+               unopt => <<~'EOF',
+                       (000) ldb      [1]
+                       (001) jeq      #0xa4            jt 2    jf 3
+                       (002) ret      #262144
+                       (003) ret      #0
+                       EOF
+       }, # arcnet_dst_host

        fddi_broadcast => {
                DLT => 'FDDI',
                expr => 'broadcast',
        fddi_broadcast => {
                DLT => 'FDDI',
                expr => 'broadcast',


@@ -5942,6 +5984,26 @@ my %apply_blocks = (
                expr => 'sls != 9',
                results => [0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
        },
                expr => 'sls != 9',
                results => [0, 0, 0, 0, 0, 0, 0, 0, 0, 0],
        },

+       link_host_c0_on_arcnet => {
+               savefile => 'bacnet-arcnet-linux.pcap',
+               expr => 'link host $c0',
+               results => [65535, 65535, 0, 65535, 65535, 65535, 65535, 65535, 65535, 0],
+       },
+       link_src_host_c0_on_arcnet => {
+               savefile => 'bacnet-arcnet-linux.pcap',
+               expr => 'link src host $c0',
+               results => [65535, 65535, 0, 65535, 0, 65535, 0, 65535, 0, 0],
+       },
+       link_dst_host_c0_on_arcnet => {
+               savefile => 'bacnet-arcnet-linux.pcap',
+               expr => 'link dst host $c0',
+               results => [0, 0, 0, 0, 65535, 0, 65535, 0, 65535, 0],
+       },
+       link_host_not_c0_on_arcnet => {
+               savefile => 'bacnet-arcnet-linux.pcap',
+               expr => 'link host not $c0',
+               results => [0, 0, 65535, 0, 0, 0, 0, 0, 0, 65535],
+       },

 );
 
 # * DLT, expr and skip: same as in accept_blocks above
 );
 
 # * DLT, expr and skip: same as in accept_blocks above


@@ -6226,6 +6288,26 @@ my %reject_tests = (
                expr => 'ifindex 1',
                errstr => 'not supported',
        },
                expr => 'ifindex 1',
                errstr => 'not supported',
        },

+       arcnet_address1 => {
+               DLT => 'ARCNET',
+               expr => 'link host $123',
+               errstr => 'syntax error',
+       },
+       arcnet_address2 => {
+               DLT => 'ARCNET',
+               expr => 'link host $x',
+               errstr => 'syntax error',
+       },
+       arcnet_address3 => {
+               DLT => 'ARCNET',
+               expr => 'link host $',
+               errstr => 'syntax error',
+       },
+       arcnet_address4 => {
+               DLT => 'ARCNET',
+               expr => 'link host 120',
+               errstr => 'illegal link layer address',
+       },

 );
 
 # On all platforms where timeout(1) is available it exits with status 124
 );
 
 # On all platforms where timeout(1) is available it exits with status 124

diff --git a/tests/filter/bacnet-arcnet-linux.pcap b/tests/filter/bacnet-arcnet-linux.pcap
new file mode 100644 (file)
index 0000000..424ce4d
Binary files /dev/null and b/tests/filter/bacnet-arcnet-linux.pcap differ