Various bugs/issues for discussion

[codedance]

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]

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

[cdevr]

Wow, what a brilliant bug report ! Thanks. I am ashamed to admit, it explains a few things.

Issue number 3 has been fixed. This was actually a problem we encountered in production. I guess I read the standard and interpreted things a little too literally. It says all oid's start with .1.3. Wrong.

[cdevr]

I'm hoping 39d42da fixes the GetTable problem.

[cdevr]

I have followed suggestions 1 and 2.

[codedance]

Thanks for picking up the issues. I've been working with Sonia on her branch of GoSNMP and we're added quite a few new features: https://github.com/soniah/gosnmp

The more and more I dive into SNMP the more corner cases we come across. There are a lot of agent implementations out there and they all have their idiosyncrasy. It's hard work!

I like many of the concepts in WapSNMP and are looking at borrowing a few such as your concept of MustParseOid() and centralising the BER parsing code. I'm not sure what your plans are for the project. The two code bases are quite different, but l'm sure sharing knowledge would be well worth it.

[cdevr]

Wow you've been hard at work on that branch. I'm hoping the project can become a full SNMP implementation for Go. Lots of things are being implemented in various branches, and I'd love to bring them together. I was hoping you guys would sign the Google CLA, and I would be able to really integrate your changes.

https://developers.google.com/open-source/cla/individual?csw=1

Do you think working like this would be possible ?

[codedance]

Personally I don't have any issue in signing the agreement. Is this because the code is internally used in Google?

[cdevr]

Well it's standard procedure for code that is maintained and used within Google. I don't know the exact details however.