diff --git a/src/net_helper.c b/src/net_helper.c index d840446076f9..c50aee66192e 100644 --- a/src/net_helper.c +++ b/src/net_helper.c @@ -110,7 +110,7 @@ static int sample_conv_eth_vlan(const struct arg *arg_p, struct sample *smp, voi smp->flags &= ~SMP_F_CONST; return !!vlan; } - if (idx + 4 < smp->data.u.str.data) + if (smp->data.u.str.data < idx + 4) break; vlan = read_n16(smp->data.u.str.area + idx + 2) & 0xfff; @@ -435,7 +435,7 @@ static int tcp_fullhdr_length(const struct sample *smp) } /* returns the offset in the input TCP header where option kind is first - * seen, otherwise 0 if not found. NOP and END cannot be searched. + * seen, otherwise 0 if not found. END (kind 0) cannot be searched. */ static size_t tcp_fullhdr_find_opt(const struct sample *smp, uint8_t opt) { @@ -813,7 +813,7 @@ static int sample_conv_ip_fp(const struct arg *arg_p, struct sample *smp, void * write_n16(trash->area + 3, tcpwin); write_n16(trash->area + 5, tcpmss); - /* the the bit mask of present options */ + /* then the bit mask of present options */ trash->area[7] = opts; /* mode 4: append source IP address */ @@ -823,7 +823,7 @@ static int sample_conv_ip_fp(const struct arg *arg_p, struct sample *smp, void * trash->data += iplen; } - /* option kinds if any are stored starting at offset 7 */ + /* option kinds if any are stored starting at offset 8 */ smp->data.u.str = *trash; smp->flags &= ~SMP_F_CONST; return 1; diff --git a/tests/unit/test-eth-vlan.c b/tests/unit/test-eth-vlan.c new file mode 100644 index 000000000000..2e0e11f0c3a0 --- /dev/null +++ b/tests/unit/test-eth-vlan.c @@ -0,0 +1,242 @@ +/* test-eth-vlan.c: unit tests for sample_conv_eth_vlan() + * + * The function under test is sample_conv_eth_vlan() from src/net_helper.c. + * Rather than pull in HAProxy's sample/arg machinery we reproduce the + * self-contained logic here so the test compiles with just: + * + * gcc -Iinclude -Wall -W -o test-eth-vlan tests/unit/test-eth-vlan.c + * + * A previous version of the function had the condition: + * if (idx + 4 < smp->data.u.str.data) break; <-- BUG: inverted sense + * which caused it to break out of the loop when there WAS enough room to + * continue, making it impossible to ever read a VLAN TCI value. The fix: + * if (smp->data.u.str.data < idx + 4) break; <-- CORRECT + * These tests document the expected behaviour and catch any regression. + * + * NOTE on the loop guard: the function checks (idx + 2 < data) before + * reading two bytes at area[idx]. This means reading a 14-byte untagged + * frame requires data >= 15 to satisfy (12 + 2 < data). Tests use the + * minimum data value that allows correct operation. + */ + +#include +#include +#include +#include +#include + +/* Minimal stand-in for struct sample. + * We only need the fields accessed by sample_conv_eth_vlan: + * data.u.str.{area,data} -- input buffer + * data.u.sint -- output VLAN ID (overlaps str in the union) + * data.type -- set to SMP_T_SINT on success + * flags -- SMP_F_CONST bit cleared on success + */ +#define SMP_T_SINT 1 +#define SMP_F_CONST (1 << 2) + +struct smp_data { + int type; + union { + struct { char *area; size_t data; } str; + long long sint; + } u; +}; + +struct sample { + struct smp_data data; + unsigned int flags; +}; + +/* ---- Re-implementation of sample_conv_eth_vlan (the fixed version) -------- */ + +static int eth_vlan(struct sample *smp) +{ + ushort vlan = 0; + size_t idx; + + for (idx = 12; idx + 2 < smp->data.u.str.data; idx += 4) { + if (read_n16(smp->data.u.str.area + idx) != 0x8100) { + smp->data.u.sint = vlan; + smp->data.type = SMP_T_SINT; + smp->flags &= ~SMP_F_CONST; + return !!vlan; + } + if (smp->data.u.str.data < idx + 4) + break; + + vlan = read_n16(smp->data.u.str.area + idx + 2) & 0xfff; + } + /* incomplete header */ + return 0; +} + +/* Helper: write a big-endian 16-bit value at byte offset in . */ +static void put16(char *buf, size_t off, uint16_t v) +{ + buf[off] = (v >> 8) & 0xff; + buf[off + 1] = v & 0xff; +} + +#define BUF_SIZE 32 + +/* ---- Individual tests ----------------------------------------------------- */ + +/* Buffer too short to contain an ethertype: the loop guard (idx + 2 < data) + * with idx=12 requires data >= 15; here data=12 → never enters → return 0. */ +static int test_too_short(void) +{ + char buf[BUF_SIZE] = {0}; + struct sample smp = { .data = { .u = { .str = { buf, 12 } } } }; + + if (eth_vlan(&smp) != 0) + return __LINE__; + return 0; +} + +/* Untagged frame: ethertype at offset 12 is not 0x8100. The loop enters + * (data=15 satisfies 12+2 < 15) and immediately takes the return branch with + * vlan=0, so !!vlan = 0. */ +static int test_no_vlan(void) +{ + char buf[BUF_SIZE] = {0}; + struct sample smp = { .data = { .u = { .str = { buf, 15 } } } }; + int ret; + + put16(buf, 12, 0x0800); /* IPv4 ethertype */ + + ret = eth_vlan(&smp); + if (ret != 0) + return __LINE__; + /* sint is set to 0 (= vlan) on this path */ + if (smp.data.u.sint != 0) + return __LINE__; + return 0; +} + +/* Single-tagged frame: 0x8100 at offset 12, TCI=42 at offset 14, IPv4 at 16. + * data=19 satisfies idx+2=18 < 19 so the inner-ethertype iteration runs. + * Return value must be 1, sint must be 42. */ +static int test_single_vlan(void) +{ + char buf[BUF_SIZE] = {0}; + struct sample smp = { .data = { .u = { .str = { buf, 19 } } } }; + int ret; + + put16(buf, 12, 0x8100); /* 802.1Q tag type */ + put16(buf, 14, 42); /* TCI: VLAN 42 */ + put16(buf, 16, 0x0800); /* inner ethertype: IPv4 */ + + ret = eth_vlan(&smp); + if (ret != 1) + return __LINE__; + if (smp.data.u.sint != 42) + return __LINE__; + return 0; +} + +/* Double-tagged (QinQ) frame: outer 0x8100 at 12, outer TCI 100 at 14, + * inner 0x8100 at 16, inner TCI 200 at 18, IPv4 at 20. + * data=23 (satisfies 20+2 < 23) lets the last iteration run. + * The function returns the *last* (innermost) VLAN ID (200). */ +static int test_double_vlan(void) +{ + char buf[BUF_SIZE] = {0}; + struct sample smp = { .data = { .u = { .str = { buf, 23 } } } }; + int ret; + + put16(buf, 12, 0x8100); /* outer 802.1Q */ + put16(buf, 14, 100); /* outer VLAN 100 */ + put16(buf, 16, 0x8100); /* inner 802.1Q */ + put16(buf, 18, 200); /* inner VLAN 200 */ + put16(buf, 20, 0x0800); /* inner ethertype: IPv4 */ + + ret = eth_vlan(&smp); + if (ret != 1) + return __LINE__; + if (smp.data.u.sint != 200) + return __LINE__; + return 0; +} + +/* Frame truncated right after the 0x8100 ethertype: data=15 allows the loop + * to enter (14 < 15), reads 0x8100, then checks (data < idx + 4) → 15 < 16 + * which is TRUE → breaks → incomplete → return 0. */ +static int test_truncated_after_8100(void) +{ + char buf[BUF_SIZE] = {0}; + struct sample smp = { .data = { .u = { .str = { buf, 15 } } } }; + + put16(buf, 12, 0x8100); + /* TCI bytes at 14-15 are only partially present (data ends at 15) */ + + if (eth_vlan(&smp) != 0) + return __LINE__; + return 0; +} + +/* VLAN tag present but no inner ethertype: 0x8100 at 12, TCI=7 at 14, + * data=16. Loop enters at idx=12 (14 < 16), reads TCI, advances idx to 16, + * then loop guard (16+2=18 < 16) fails → exits → incomplete → return 0. */ +static int test_vlan_no_inner_ethertype(void) +{ + char buf[BUF_SIZE] = {0}; + struct sample smp = { .data = { .u = { .str = { buf, 16 } } } }; + + put16(buf, 12, 0x8100); + put16(buf, 14, 7); /* VLAN 7 */ + /* no inner ethertype – only 16 bytes total */ + + if (eth_vlan(&smp) != 0) + return __LINE__; + return 0; +} + +/* Regression test for the inverted-break bug. + * + * The old code read: + * if (idx + 4 < smp->data.u.str.data) break; + * + * With a single-VLAN frame and data=19: + * idx=12, idx+4=16, 16 < 19 → TRUE → the buggy code broke immediately, + * so vlan was never read from TCI and sint was never set, always returning 0. + * + * With the fix: + * if (smp->data.u.str.data < idx + 4) break; → 19 < 16 → FALSE → continues + * and the function correctly reads the VLAN ID and returns 1. + */ +static int test_regression_inverted_break(void) +{ + char buf[BUF_SIZE] = {0}; + struct sample smp = { .data = { .u = { .str = { buf, 19 } } } }; + int ret; + + put16(buf, 12, 0x8100); + put16(buf, 14, 42); + put16(buf, 16, 0x0800); + + ret = eth_vlan(&smp); + + /* The buggy version returned 0 here; the correct version returns 1. */ + if (ret != 1) + return __LINE__; + if (smp.data.u.sint != 42) + return __LINE__; + return 0; +} + +int main(void) +{ + int tret = 0; + int fret = 0; + + tret = test_too_short(); printf("%4d test_too_short()\n", tret); if (!fret) fret = tret; + tret = test_no_vlan(); printf("%4d test_no_vlan()\n", tret); if (!fret) fret = tret; + tret = test_single_vlan(); printf("%4d test_single_vlan()\n", tret); if (!fret) fret = tret; + tret = test_double_vlan(); printf("%4d test_double_vlan()\n", tret); if (!fret) fret = tret; + tret = test_truncated_after_8100(); printf("%4d test_truncated_after_8100()\n", tret); if (!fret) fret = tret; + tret = test_vlan_no_inner_ethertype(); printf("%4d test_vlan_no_inner_ethertype()\n", tret); if (!fret) fret = tret; + tret = test_regression_inverted_break(); printf("%4d test_regression_inverted_break()\n", tret); if (!fret) fret = tret; + + return !!fret; +} diff --git a/tests/unit/test-eth-vlan.sh b/tests/unit/test-eth-vlan.sh new file mode 100644 index 000000000000..54f94c598a6e --- /dev/null +++ b/tests/unit/test-eth-vlan.sh @@ -0,0 +1,28 @@ +#!/bin/sh + +check() { + : +} + +run() { + gcc -Iinclude -Wall -W -fomit-frame-pointer -Os \ + ${ROOTDIR}/tests/unit/test-eth-vlan.c \ + -o ${ROOTDIR}/tests/unit/test-eth-vlanOs + ${ROOTDIR}/tests/unit/test-eth-vlanOs + rm ${ROOTDIR}/tests/unit/test-eth-vlanOs + + gcc -Iinclude -Wall -W -fomit-frame-pointer -O2 \ + ${ROOTDIR}/tests/unit/test-eth-vlan.c \ + -o ${ROOTDIR}/tests/unit/test-eth-vlanO2 + ${ROOTDIR}/tests/unit/test-eth-vlanO2 + rm ${ROOTDIR}/tests/unit/test-eth-vlanO2 +} + +case "$1" in + "check") + check + ;; + "run") + run + ;; +esac