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 2f3bc551ad1cba0891a97e4c8fe9c143b45de56e..d8839267f4849814c1fab31c1470adbe95e9ecb9 100644 (file)
--- 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 39560c26124ff5626e8e7320024658d57cf2a974..2fd1a4a9a1e51ccc7fd64f0a136c7230faac1c8c 100644 (file)
--- 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 7468e3f4c8a9ad46e873b6b319b44502e9a2d10f..e0146356595c06238e85b6c0b95babeca48240a6 100644 (file)
--- 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 (file)
index 0000000..a20aa12
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 15dc91c2e39f254d02ea4dd1a6110c84ebb0bf11..6724e8a83ade66fe7588bb85c6e24faaffdd76f2 100644 (file)
--- 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 4befc48559fde297fc17b0ebcea16e5723e05660..4ecf7b57381076885b55dd739b69d2fac56aee33 100644 (file)
--- 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 6eedded63b8cb344d9d6d5709b9f0bc5ca316577..42a2cd66ae6361643de8d8afbca3b53241b444ff 100644 (file)
--- 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")