On Tue, Oct 08, 2019 at 09:36:27AM +0200, Martijn van Duren wrote:
> ping. I think this (or similar) fix should go in before release.
>
> On 10/3/19 5:33 PM, Martijn van Duren wrote:
> > As discussed with semarie@ this morning: We're a bit too loose when it
> > comes to accessing indexes based directly from the pdu.
> >
> > Diff below allows only indexes within the range of the pdu.
> > If user supplied oid and returned oid on index match the user supplied
> > input is shown (for easier reference). If they don't match then show
> > the oid based on the -O flag. If the index is out of range throw an
> > additional error message and show a "?".
> >
> > Since it apparently is hard to get a proper error status with the
> > daemons available to me, this is only compile tested. Testers welcome.
> >
> > This also adds a missing NULL-check, which could be committed
> > separately.
> >
> > OK?
A little bit of bikeshedding below.
> > martijn@
> >
> > Index: snmpc.c
> > ===================================================================
> > RCS file: /cvs/src/usr.bin/snmp/snmpc.c,v
> > retrieving revision 1.13
> > diff -u -p -r1.13 snmpc.c
> > --- snmpc.c 3 Oct 2019 11:02:26 -0000 1.13
> > +++ snmpc.c 3 Oct 2019 15:30:50 -0000
> > @@ -52,7 +52,8 @@ int snmpc_mibtree(int, char *[]);
> > struct snmp_agent *snmpc_connect(char *, char *);
> > int snmpc_parseagent(char *, char *);
> > int snmpc_print(struct ber_element *);
> > -__dead void snmpc_printerror(enum snmp_error, char *);
> > +__dead void snmpc_printerror(enum snmp_error, struct ber_element *, int,
> > + const char *);
> > char *snmpc_hex2bin(char *, size_t *);
> > struct ber_element *snmpc_varbindparse(int, char *[]);
> > void usage(void);
> > @@ -480,6 +481,7 @@ snmpc_get(int argc, char *argv[])
> > int i;
> > int class;
> > unsigned type;
> > + char *hint = NULL;
> >
> > if (argc < 2)
> > usage();
> > @@ -519,9 +521,12 @@ snmpc_get(int argc, char *argv[])
> >
> > (void) ber_scanf_elements(pdu, "t{Sdd{e", &class, &type, &errorstatus,
> > &errorindex, &varbind);
> > - if (errorstatus != 0)
> > - snmpc_printerror((enum snmp_error) errorstatus,
> > - argv[errorindex - 1]);
> > + if (errorstatus != 0) {
> > + if (errorindex >= 1 && errorindex <= argc)
> > + hint = argv[errorindex - 1];
> > + snmpc_printerror((enum snmp_error) errorstatus, varbind,
> > + errorindex, hint);
> > + }
> >
> > if (class == BER_CLASS_CONTEXT && type == SNMP_C_REPORT)
> > printf("Received report:\n");
> > @@ -542,7 +547,6 @@ snmpc_walk(int argc, char *argv[])
> > struct timespec start, finish;
> > struct snmp_agent *agent;
> > const char *oids;
> > - char oidstr[SNMP_MAX_OID_STRLEN];
> > int n = 0, prev_cmp;
> > int errorstatus, errorindex;
> > int class;
> > @@ -574,8 +578,8 @@ snmpc_walk(int argc, char *argv[])
> > (void) ber_scanf_elements(pdu, "t{Sdd{e", &class, &type,
> > &errorstatus, &errorindex, &varbind);
> > if (errorstatus != 0)
> > - snmpc_printerror((enum snmp_error) errorstatus,
> > - argv[errorindex - 1]);
> > + snmpc_printerror((enum snmp_error) errorstatus, varbind,
> > + errorindex, oids);
> >
> > if (class == BER_CLASS_CONTEXT && type == SNMP_C_REPORT)
> > printf("Received report:\n");
> > @@ -600,9 +604,8 @@ snmpc_walk(int argc, char *argv[])
> > (void) ber_scanf_elements(pdu, "t{Sdd{e", &class, &type,
> > &errorstatus, &errorindex, &varbind);
> > if (errorstatus != 0) {
> > - smi_oid2string(&noid, oidstr, sizeof(oidstr),
> > - oid_lookup);
> > - snmpc_printerror((enum snmp_error) errorstatus, oidstr);
> > + snmpc_printerror((enum snmp_error) errorstatus, varbind,
> > + errorindex, NULL);
> > }
> >
> > if (class == BER_CLASS_CONTEXT && type == SNMP_C_REPORT)
> > @@ -639,8 +642,8 @@ snmpc_walk(int argc, char *argv[])
> > (void) ber_scanf_elements(pdu, "t{Sdd{e", &class, &type,
> > &errorstatus, &errorindex, &varbind);
> > if (errorstatus != 0)
> > - snmpc_printerror((enum snmp_error) errorstatus,
> > - argv[errorindex - 1]);
> > + snmpc_printerror((enum snmp_error) errorstatus, varbind,
> > + errorindex, oids);
> >
> > if (class == BER_CLASS_CONTEXT && type == SNMP_C_REPORT)
> > printf("Received report:\n");
> > @@ -676,6 +679,7 @@ snmpc_set(int argc, char *argv[])
> > int errorstatus, errorindex;
> > int class;
> > unsigned type;
> > + char *hint = NULL;
> >
> > if (argc < 4)
> > usage();
> > @@ -687,13 +691,17 @@ snmpc_set(int argc, char *argv[])
> > if (pledge("stdio", NULL) == -1)
> > err(1, "pledge");
> >
> > - pdu = snmp_set(agent, snmpc_varbindparse(argc, argv));
> > + if ((pdu = snmp_set(agent, snmpc_varbindparse(argc, argv))) == NULL)
> > + err(1, "set");
> >
> > (void) ber_scanf_elements(pdu, "t{Sdd{e", &class, &type, &errorstatus,
> > &errorindex, &varbind);
> > - if (errorstatus != 0)
> > - snmpc_printerror((enum snmp_error) errorstatus,
> > - argv[errorindex - 1]);
> > + if (errorstatus != 0) {
> > + if (errorindex >= 1 && errorindex <= argc / 3)
> > + hint = argv[(errorindex - 1) * 3];
> > + snmpc_printerror((enum snmp_error) errorstatus, varbind,
> > + errorindex, hint);
> > + }
> >
> > if (class == BER_CLASS_CONTEXT && type == SNMP_C_REPORT)
> > printf("Received report:\n");
> > @@ -805,8 +813,36 @@ snmpc_print(struct ber_element *elm)
> > }
> >
> > __dead void
> > -snmpc_printerror(enum snmp_error error, char *oid)
> > +snmpc_printerror(enum snmp_error error, struct ber_element *varbind,
> > + int index, const char *hint)
> > {
> > + struct ber_oid hoid, vboid;
> > + char oids[SNMP_MAX_OID_STRLEN];
> > + const char *oid = NULL;
> > + int i;
> > +
> > + if (index >= 1) {
> > + /* Only print if the index is in the reply */
> > + for (i = 1; varbind != NULL && i <= index;
> > + varbind = varbind->be_next)
> > + i++;
> > + if (varbind != NULL &&
> > + ber_get_oid(varbind->be_sub, &vboid) == 0) {
> > + /* If user and reply conform print user input */
> > + if (smi_string2oid(hint, &hoid) == 0 &&
> > + ber_oid_cmp(&hoid, &vboid) == 0 &&
> > + hint != NULL)
Why not move the hint != NULL to the start of that if condition. It is the
quickest check.
> > + oid = hint;
> > + else
> > + oid = smi_oid2string(&vboid, oids,
> > + sizeof(oids), oid_lookup);
> > + }
> > + }
> > + if (oid == NULL) {
> > + fprintf(stderr, "Invalid varbind error-index received\n");
Isn't the '?' for OID enough? Do we really need this extra fprintf here?
> > + oid = "?";
> > + }
> > +
> > switch (error) {
> > case SNMP_ERROR_NONE:
> > errx(1, "No error, how did I get here?");
> >
Apart from that OK claudio@
--
:wq Claudio
