snmp(1): Better error reporting on malformed packets

[Martijn van Duren]

Right now if we receive a malformed reply (apart from potentially
crashing[0]) we return a rather unsightly and uninformative error
message:
$ LD_PRELOAD=/usr/src/lib/libutil/obj/libutil.so.13.1 snmp getnext -v2c 
-cpublic 127.0.0.1 ifInDiscards.0      
snmp: getnext: Undefined error: 0

This diff always sets errno to EPROTO if we can't parse the packet.
$ LD_PRELOAD=/usr/src/lib/libutil/obj/libutil.so.13.1 ./obj/snmp getnext -v2c 
-cpublic 127.0.0.1 ifInDiscards.0
snmp: getnext: Protocol error

(note that I malformed snmpd to return "{o}" for this case).

This diff also retries the packet if a malformed reply is received.
This is similar to what netsnmp does.

OK?

martijn@

[0] https://marc.info/?l=openbsd-tech&m=156570856731073&w=2

Index: snmp.c
===================================================================
RCS file: /cvs/src/usr.bin/snmp/snmp.c,v
retrieving revision 1.1
diff -u -p -r1.1 snmp.c
--- snmp.c      9 Aug 2019 06:17:59 -0000       1.1
+++ snmp.c      13 Aug 2019 17:25:28 -0000
@@ -254,26 +254,51 @@ snmp_resolve(struct snmp_agent *agent, s
                if (ret <= 0)
                        goto fail;
                ber_set_readbuf(&ber, buf, ret);
-               if ((message = ber_read_elements(&ber, NULL)) == NULL)
-                       goto fail;
+               if ((message = ber_read_elements(&ber, NULL)) == NULL) {
+                       direction = POLLOUT;
+                       tries--;
+                       continue;
+               }
                if (ber_scanf_elements(message, "{ise", &version, &community,
-                   &pdu) != 0)
-                       goto fail;
+                   &pdu) != 0) {
+                       errno = EPROTO;
+                       direction = POLLOUT;
+                       tries--;
+                       continue;
+               }
                /* Skip invalid packets; should not happen */
                if (version != agent->version ||
-                   strcmp(community, agent->community) != 0)
+                   strcmp(community, agent->community) != 0) {
+                       errno = EPROTO;
+                       direction = POLLOUT;
+                       tries--;
                        continue;
+               }
                /* Validate pdu format and check request id */
                if (ber_scanf_elements(pdu, "{iSSe", &rreqid, &varbind) != 0 ||
-                   varbind->be_encoding != BER_TYPE_SEQUENCE)
-                       goto fail;
-               if (rreqid != reqid)
+                   varbind->be_encoding != BER_TYPE_SEQUENCE) {
+                       errno = EPROTO;
+                       direction = POLLOUT;
+                       tries--;
                        continue;
+               }
+               if (rreqid != reqid) {
+                       errno = EPROTO;
+                       direction = POLLOUT;
+                       tries--;
+                       continue;
+               }
                for (varbind = varbind->be_sub; varbind != NULL;
                    varbind = varbind->be_next) {
-                       if (ber_scanf_elements(varbind, "{oS}", &oid) != 0)
-                               goto fail;
+                       if (ber_scanf_elements(varbind, "{oS}", &oid) != 0) {
+                               errno = EPROTO;
+                               direction = POLLOUT;
+                               tries--;
+                               break;
+                       }
                }
+               if (varbind != NULL)
+                       continue;
 
                ber_unlink_elements(message->be_sub->be_next);
                ber_free_elements(message);