session.walk method breaks within the library

[devqazi]

How to reproduce

1. Have some agents running on your machine. (I used SNMP Agent simulator)
2. Use the walk method like below

   /* Some code excluded for brevity */
   const session = snmp.createSession('127.0.0.1', 'public');
   session.walk ('1.3.6.1.4.1', maxRepetitions, feedCb, doneCb);

3. Running it will give out an error after some data is fetched.

   
   C:\Users\<~redacted~>\Documents\<~redacted~>\node_modules\net-snmp\index.js:2645
       if ( ! varbinds.length ) {
                       ^

TypeError: Cannot read property 'length' of undefined at Session.walkCb (C:\Users\Sayam\Documents\fiverr\docksters\docksters-collect\node_modules\net-snmp\index.js:2645:18) at Req.Session.onSimpleGetResponse [as onResponse] (C:\Users\Sayam\Documents\fiverr\docksters\docksters-collect\node_modules\net-snmp\index.js:2224:8) at Session.onMsg (C:\Users\Sayam\Documents\fiverr\docksters\docksters-collect\node_modules\net-snmp\index.js:2202:7) at Socket.emit (events.js:315:20) at UDP.onMessage [as onmessage] (dgram.js:910:8)

#### Findings
I found this in the library code in `index.js` line `2645`

if ( ! varbinds.length ) { req.doneCb(null); return; }

A simple fix would be to make it

if (!varbinds || !varbinds.length) { } // or if (!(varbinds && varbinds.length)) { }

Note that the `feedCb` is fired a few times so It works for a while then breaks. 

[markabrahams]

Hi @devqazi - I have just run your code against the Net-SNMP agent, and the agent included with this library and they both work fine. The problem could be that whatever SNMP agent simulator you're using is not conforming to standards in its use of the SNMP protocol, if the failure is peculiar to this simulator. You could confirm that by looking at packet captures for the failure scenario.

Also, I've added your suggested fix, as it wouldn't appear to adversely affect the current SNMP walk behaviour, and a library crash is undesirable, irrespective of the behaviour of the agent! The fix is on master now, and published in version 3.5.7 of the npm.

[devqazi]

Yes and I kind of think that something that is coming from external source should be properly validated regardless of what you expect it to be. So I felt like that check should have been there all along regardless of the protocol. I tend to think of writing any processing function like this.

Under no circumstances the external source should be able to produce an input which if passed down to my program it just simply breaks. Instead it should be able to deny the process for "invalid/unexpected" inputs and have a predefined behavior in that case.

Huge thanks for the quick fix though.