From: Vincent Bernat Date: Sat, 23 May 2020 12:32:39 +0000 (+0200) Subject: agent: fix SNMP walk on lldpRemTable when missing remote sysName X-Git-Tag: 1.0.6~18^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=351a4b1d149ac2d7445f3e7e7748dff345161817;p=thirdparty%2Flldpd.git agent: fix SNMP walk on lldpRemTable when missing remote sysName When enumerating lldpRemSysName (and some others), one row could have a NULL value because the remote system didn't provide a value. In this case, we should return the next row. There was already some code around that but it was not systematically used. Therefore, we fix the issue for lldpRemTable and lldpLocalSystemData. To ensure we catch future cases, we ensure helpers functions use `default: return NULL` when no missing value is allowed (no `break`, compiler would catch if it was the case) and therefore, we don't need to try next OID and `default: break` when a value may be missing and in this case, the caller should try next OID upon receiving NULL. Fix #392 --- diff --git a/NEWS b/NEWS index 2f3bc551..d8839267 100644 --- a/NEWS +++ b/NEWS @@ -1,6 +1,8 @@ lldpd (1.0.6) * Fix: + Do not loose chassis local information when interface status changes. + + Fix SNMP walk on lldpRemTable when missing remote system + name or description. * Changes: + Deprecate use of lldpctl_watch_callback(). Use lldpctl_watch_callback2() instead. diff --git a/src/daemon/agent.c b/src/daemon/agent.c index 39560c26..2fd1a4a9 100644 --- a/src/daemon/agent.c +++ b/src/daemon/agent.c @@ -1048,22 +1048,29 @@ static u_char* agent_h_local_chassis(struct variable *vp, oid *name, size_t *length, int exact, size_t *var_len, WriteMethod **write_method) { + u_char *a; + if (header_generic(vp, name, length, exact, var_len, write_method)) return NULL; - return agent_v_chassis(vp, var_len, LOCAL_CHASSIS(scfg)); + if ((a = agent_v_chassis(vp, var_len, LOCAL_CHASSIS(scfg))) != NULL) + return a; + TRYNEXT(agent_h_local_chassis); } static u_char* agent_h_remote_chassis(struct variable *vp, oid*name, size_t *length, int exact, size_t *var_len, WriteMethod **write_method) { struct lldpd_port *port; + u_char *a; if ((port = header_tprindexed_table(vp, name, length, exact, var_len, write_method, 0)) == NULL) return NULL; - return agent_v_chassis(vp, var_len, port->p_chassis); + if ((a = agent_v_chassis(vp, var_len, port->p_chassis)) != NULL) + return a; + TRYNEXT(agent_h_remote_chassis); } static u_char* @@ -1100,9 +1107,8 @@ agent_h_stats(struct variable *vp, oid *name, size_t *length, long_ret = hardware->h_ageout_cnt; return (u_char *)&long_ret; default: - break; + return NULL; } - return NULL; } #ifdef ENABLE_DOT1 @@ -1114,9 +1120,8 @@ agent_v_vlan(struct variable *vp, size_t *var_len, struct lldpd_vlan *vlan) *var_len = strlen(vlan->v_name); return (u_char *)vlan->v_name; default: - break; + return NULL; } - return NULL; } static u_char* agent_h_local_vlan(struct variable *vp, oid *name, size_t *length, @@ -1156,9 +1161,8 @@ agent_v_ppvid(struct variable *vp, size_t *var_len, struct lldpd_ppvid *ppvid) long_ret = (ppvid->p_cap_status & LLDP_PPVID_CAP_ENABLED)?1:2; return (u_char *)&long_ret; default: - break; + return NULL; } - return NULL; } static u_char* agent_h_local_ppvid(struct variable *vp, oid *name, size_t *length, @@ -1194,14 +1198,13 @@ agent_v_pi(struct variable *vp, size_t *var_len, struct lldpd_pi *pi) *var_len = pi->p_pi_len; return (u_char *)pi->p_pi; default: - break; + return NULL; } - return NULL; } static u_char* agent_h_local_pi(struct variable *vp, oid *name, size_t *length, int exact, size_t *var_len, WriteMethod **write_method) -{ +{ struct lldpd_pi *pi; if ((pi = header_ppiindexed_table(vp, name, length, @@ -1413,9 +1416,8 @@ agent_v_management(struct variable *vp, size_t *var_len, struct lldpd_mgmt *mgmt *var_len = sizeof(zeroDotZero); return (u_char*)zeroDotZero; default: - break; + return NULL; } - return NULL; } static u_char* agent_h_local_management(struct variable *vp, oid *name, size_t *length, @@ -1452,9 +1454,8 @@ agent_v_custom(struct variable *vp, size_t *var_len, struct lldpd_custom *custom *var_len = custom->oui_info_len; return (u_char *)custom->oui_info; default: - break; + return NULL; } - return NULL; } static u_char* agent_h_remote_custom(struct variable *vp, oid *name, size_t *length, diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 7468e3f4..e0146356 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -1,4 +1,8 @@ +pytest_plugins = ['helpers_namespace'] + import pytest +import scapy.all + from fixtures.programs import * from fixtures.namespaces import * from fixtures.network import * @@ -13,3 +17,10 @@ def root(): # _lldpd. Just do a plain namespace. with Namespace('pid', 'net', 'mnt', 'ipc', 'uts'): yield + + +@pytest.helpers.register +def send_pcap(location, interface): + packets = scapy.all.rdpcap(location) + print(packets) + scapy.all.sendp(packets, iface=interface) diff --git a/tests/integration/data/connectx.pcap b/tests/integration/data/connectx.pcap new file mode 100644 index 00000000..a20aa12c Binary files /dev/null and b/tests/integration/data/connectx.pcap differ diff --git a/tests/integration/requirements.txt b/tests/integration/requirements.txt index 15dc91c2..6724e8a8 100644 --- a/tests/integration/requirements.txt +++ b/tests/integration/requirements.txt @@ -1,4 +1,5 @@ pyroute2==0.5.2 pytest==3.10.1 +pytest-helpers-namespace==2019.1.8 pytest-xdist==1.26.1 scapy==2.4.3 diff --git a/tests/integration/test_pcap.py b/tests/integration/test_pcap.py index 4befc485..4ecf7b57 100644 --- a/tests/integration/test_pcap.py +++ b/tests/integration/test_pcap.py @@ -1,16 +1,9 @@ import pytest -import scapy.all - - -def send_pcap(location, interface): - packets = scapy.all.rdpcap(location) - print(packets) - scapy.all.sendp(packets, iface=interface) def test_cisco_sg200(request, lldpd1, lldpcli, namespaces): with namespaces(2): - send_pcap('data/sg200.pcap', 'eth1') + pytest.helpers.send_pcap('data/sg200.pcap', 'eth1') with namespaces(1): out = lldpcli("-f", "keyvalue", "show", "neighbors", "details") assert out['lldp.eth0.age'].startswith('0 day, 00:00:') @@ -59,7 +52,7 @@ def test_cisco_sg200(request, lldpd1, lldpcli, namespaces): readon="Dot3 not supported") def test_8023bt(lldpd1, lldpcli, namespaces): with namespaces(2): - send_pcap('data/8023bt.pcap', 'eth1') + pytest.helpers.send_pcap('data/8023bt.pcap', 'eth1') with namespaces(1): out = lldpcli("-f", "keyvalue", "show", "neighbors", "details") for k in list(out.keys()): diff --git a/tests/integration/test_snmp.py b/tests/integration/test_snmp.py index 6eedded6..42a2cd66 100644 --- a/tests/integration/test_snmp.py +++ b/tests/integration/test_snmp.py @@ -1,5 +1,4 @@ import pytest -import time pytestmark = pytest.mark.skipif("not config.lldpd.snmp", reason="no SNMP support") @@ -27,3 +26,33 @@ def test_snmp_one_neighbor(snmpd, snmpwalk, lldpd, namespaces): assert out['.1.0.8802.1.1.2.1.3.3.0'] == 'STRING: "ns-1.example.com"' assert out['.1.0.8802.1.1.2.1.3.4.0'].startswith( 'STRING: "Spectacular GNU/Linux 2016 Linux') + + +def test_snmp_empty_sysname(snmpd, snmpwalk, lldpd, links, namespaces): + # See https://github.com/vincentbernat/lldpd/issues/392 + links(namespaces(1), namespaces(2)) + links(namespaces(1), namespaces(3)) + links(namespaces(1), namespaces(4)) + with namespaces(1): + snmpd() + lldpd("-x") + with namespaces(2): + lldpd() + with namespaces(3): + # Packet without sysName + lldpd("-r") + pytest.helpers.send_pcap('data/connectx.pcap', 'eth3') + with namespaces(4): + lldpd() + with namespaces(1): + out = snmpwalk(".1.0.8802.1.1.2.1.4.1.1.9") # lldpRemSysName + # We should get something like: + # .1.0.8802.1.1.2.1.4.1.1.9.400.3.1 STRING: "ns-2.example.com" + # .1.0.8802.1.1.2.1.4.1.1.9.700.5.2 (not present) + # .1.0.8802.1.1.2.1.4.1.1.9.1000.7.3 STRING: "ns-4.example.com" + print(out) + assert list(out.values()) == ['STRING: "ns-2.example.com"', + 'STRING: "ns-4.example.com"'] + oids = list(out.keys()) + assert oids[0].endswith(".3.1") + assert oids[1].endswith(".7.3")