On 10/3/19 10:56 AM, Claudio Jeker wrote:
> 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.
Nope, ber gets overwritten for each iteration, so as soon as we have the
next element we don't free pdu anymore.
>
>> 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.
So OK to get this in as is and deal with this later?
>
>> I'd say we leave this for a different patch.
>>>
>>> Apart from that OK claudio@
>>>
>>
>
