On Thu, Oct 03, 2019 at 10:42:42AM +0200, Martijn van Duren wrote:
> On 10/3/19 10:19 AM, Claudio Jeker wrote:
> > On Thu, Oct 03, 2019 at 10:01:06AM +0200, Martijn van Duren wrote:
> >> On 10/3/19 9:21 AM, Sebastien Marie wrote:
> >>> On Thu, Sep 26, 2019 at 02:33:11PM +0200, Martijn van Duren wrote:
> >>>> On 9/26/19 9:54 AM, Martijn van Duren wrote:
> >>>>> Hello,
> >>>>>
> >>>>> I reckon this will be on of the last major additions.
> >>>>> Adding "snmp set" allows us to run snmpd's regress without installing
> >>>>> netsnmp. :-)
> >>>>>
> >>>>> Tested with snmpd's regress test.
> >>>>>
> >>>>> Majority of diff is moving oid/value parsing from snmp trap to a
> >>>>> separate function.
> >>>>>
> >>>>> OK?
> >>>>>
> >>>>> martijn@
> >>>
> >>> few comments.
> >>>
> >>> thanks.
> >>>
> >>>> Index: snmpc.c
> >>>> ===================================================================
> >>>> RCS file: /cvs/src/usr.bin/snmp/snmpc.c,v
> >>>> retrieving revision 1.11
> >>>> diff -u -p -r1.11 snmpc.c
> >>>> --- snmpc.c 18 Sep 2019 09:54:36 -0000 1.11
> >>>> +++ snmpc.c 26 Sep 2019 12:32:38 -0000
> >>>> @@ -666,19 +669,54 @@ snmpc_walk(int argc, char *argv[])
> >>>> }
> >>>>
> >>>> int
> >>>> +snmpc_set(int argc, char *argv[])
> >>>> +{
> >>>> + struct snmp_agent *agent;
> >>>> + struct ber_element *pdu, *varbind;
> >>>> + int errorstatus, errorindex;
> >>>> + int class;
> >>>> + unsigned type;
> >>>> +
> >>>> + if (argc < 4)
> >>>> + usage();
> >>>> + if ((agent = snmpc_connect(argv[0], "161")) == NULL)
> >>>> + err(1, "%s", snmp_app->name);
> >>>> + argc--;
> >>>> + argv++;
> >>>> +
> >>>> + if (argc < 3 || argc % 3 != 0)
> >>>> + usage();
> >>>> +
> >>>> + if (pledge("stdio", NULL) == -1)
> >>>> + err(1, "pledge");
> >>>> +
> >>>> + pdu = snmp_set(agent, snmpc_varbindparse(argc, 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]);
> >>>
> >>> is "errorindex - 1" the right index ?
> >>
> >> Yes: RFC 3416 section 4.1:
> >> A variable binding is identified by its index value. The first variable
> >> binding in a variable-binding list is index one, the second is index
> >> two, etc.
> >>
> >> I choose to use argv, because it's easier for the end user to see his
> >> own input for where he mistyped instead of a double parsed oid to -O
> >> based output.
> >>
> >>>
> >>> $ ./obj/snmp set 192.168.1.5 sysContact.0 s "test"
> >>> snmp: Can't parse oid 192.168.1.5: Not writable
> >>>
> >>> Note that the same pattern is used in others commands.
> >>
> >> Somehow the machines I test against don't return a not writable message.
> >> Tested against snmpd, net-snmpd, HP Laserjet.
> >> Could you show me a tcpdump -v output on this output?
> >>>
> >>
> >>>> +struct ber_element *
> >>>> +snmpc_varbindparse(int argc, char *argv[])
> >>>> +{
> >>>> + struct ber_oid oid, oidval;
> >>>> + struct in_addr addr4;
> >>>> + char *addr = (char *)&addr4;
> >>>> + char *str = NULL, *tmpstr, *endstr;
> >>>> + const char *errstr = NULL;
> >>>> + struct ber_element *varbind = NULL, *vblist = NULL;
> >>>> + int i, ret;
> >>>> + size_t strl, byte;
> >>>> + long long lval;
> >>>> +
> >>>> + if (argc % 3 != 0)
> >>>> + usage();
> >>>
> >>> if I don't mess myself, callers already checks that 'argc % 3 != 0'. So I
> >>> think
> >>> the condition is more a defense for programmer error that user error, and
> >>> I
> >>> would use an assert(argc % 3 == 0) or abort().
> >>
> >> I removed these checks from snmpc_{set,trap}. Even less code. :-)
> >>
> >> Index: snmp.c
> >> ===================================================================
> >> RCS file: /cvs/src/usr.bin/snmp/snmp.c,v
> >> retrieving revision 1.6
> >> diff -u -p -r1.6 snmp.c
> >> --- snmp.c 18 Sep 2019 09:54:36 -0000 1.6
> >> +++ snmp.c 3 Oct 2019 07:59:07 -0000
> >> @@ -249,6 +249,22 @@ fail:
> >> return NULL;
> >> }
> >>
> >> +struct ber_element *
> >> +snmp_set(struct snmp_agent *agent, struct ber_element *vblist)
> >> +{
> >> + struct ber_element *pdu;
> >> +
> >> + if ((pdu = ber_add_sequence(NULL)) == NULL)
> >> + return NULL;
> >> + if (ber_printf_elements(pdu, "tddd{e", BER_CLASS_CONTEXT,
> >> + SNMP_C_SETREQ, arc4random() & 0x7fffffff, 0, 0, vblist) == NULL) {
> >> + ber_free_elements(pdu);
> >
> > Shouldn't this be ber_free_elements(vblist)?
> > If ber_printf_elements() fails pdu was already freed there.
>
> Nope, the pdu has already been allocated and is the root of this
> endeavour. So that needs to be freed.
But you pass the pdu to ber_printf_elements() and there this happens:
struct ber_element *
ber_printf_elements(struct ber_element *ber, char *fmt, ...)
{
....
fail:
ber_free_elements(ber);
return (NULL);
}
So when ber_printf_elements() returns NULL the passed pdu was already
freed up in ber_free_elements() and so the code is doing a double free.
> vblist is most likely not linked into the pdu, so we can argue if it
> should be freed separately.
>
> It's a minor leak, since we most likely are heading for the exit
> path, but it's worth thinking how we want to properly handle this (same
> for snmp_get, snmp_getnext, snmp_trap, and snmp_bulkget).
I'm fine with this and I agree that the error behaviour of
ber_printf_elements() is less than ideal since it is unclear what needs to
be cleared up and what not. This is actually a much bigger issue for the
daemons.
> I'd say we leave this for a different patch.
> >
> > Apart from that OK claudio@
> >
>
--
:wq Claudio
