Various bugs/issues for discussion

codedance commented 10 years ago

Hi Christope,

I’ve taken a quick look at the WapSNMP code and have found a few issues you may wish to take a look at:

1) ber.go - DecodeSequence

Problem: If the sequence contains EndOfMibView, an error is thrown and no values are returned. This error in turn will bubble up causing a whole GetTable or GetBulk call to fail.

Suggestion: Return EndOfMibView as a type like other types.

2) ber.go - DecodeSequence

Problem: Similar to above, hitting an unknown type at any point in a sequence, table scan, or GetMulitple will cause an error and it’s not possible to read any returned data.

Suggestion: Return an “Unsupported” type as a value, and continue, rather than raising an error at this low level. Callers can then handle on an individual OID basis as they see fit.

3) oid.go - DecodeOid

Problem: The test len(raw) < 2 is not correct. A zero length, or 0x00 (zero) OID is valid. This code failed when doing table scans on a few real world devices (e.g. Routers, NAS Servers, etc.).

Suggestion: return &Oid{}, nil

4) snmp.go - GetTable

Problem: The logic in GetTable seems to be faulty. The code to determine newLastOid assume the interaction order on the results map is deterministic. newLastOid will not be the numeric last oid, but instead a random OID (see iteration order). This bug causes very slow table walk because of overlapping pages are requested, and worse, it will randomly silently fail as there is a chance that lastOid == newLastOid.

Suggestion: I don’t think map[string] is the best data structure for this purpose. An ordered array of struct{OID, Value} may be a better choice, however this change would mean breaking the API. A non-braking solution would be to write your own ordering logic to determine the last OID (but this will be slower).

5) utils/goTable.go Problem: There seems to be a minor case issue:

-       version := wapSnmp.SNMPv2c
+       version := wapsnmp.SNMPv2c

-       oid, err := wapSnmp.ParseOid(*oidasstring)
+       oid, err := wapsnmp.ParseOid(*oidasstring)

I’d welcome your thoughts/comments on these issues, and if appropriate I can propose some fixes and/or work with you to test.

Thanks for your hard work and making SNMP first-class in Go!

codedance commented 10 years ago

As a followup, I was thinking about the name GetTable. I'm not sure it's the right name. The implementation looks more like a (sub)tree walk. Table's in SNMP/MIB have a different context. i.e.

http://www.webnms.com/snmp/help/snmpapi/snmpv3/table_handling/snmptables_basics.html