unhandled error event with session.walk about node-net-snmp HOT 23 CLOSED

It seems like this is all that's needed:

 526 var ResponseMessage = function (buffer) {
 527         var reader = new ber.Reader (buffer);
 528 
 529         reader.readSequence ();
 530 
 531         try {
 532                 this.version = reader.readInt ();
 533                 this.community = reader.readString ();
 534         } catch (err) {
 535                 throw new ResponseInvalidError("reader error " + err);
 536         }
 537 
 538         var type = reader.peek ();
 539 
 540         if (type == PduType.GetResponse) {
 541                 this.pdu = new GetResponsePdu (reader);
 542         } else {
 543                 throw new ResponseInvalidError ("Unknown PDU type '" + type
 544                                 + "' in response");
 545         }
 546 };

from node-net-snmp.

Hi @andrewhodel

This error:

InvalidAsn1Error: Expected 0x2: got 0x0

Would imply the data in the SNMP response message is not as expected.

To be able to troubleshoot this I would need the exact code you are using, and a tcpdump showing the data being parsed.

Thanks

Steve

from node-net-snmp.

Hi @stephenwvickers

This fixes it.

 526 var ResponseMessage = function (buffer) {
 527         var reader = new ber.Reader (buffer);
 528 
 529         reader.readSequence ();
 530 
 531         try {
 532                 this.version = reader.readInt ();
 533                 this.community = reader.readString ();
 534         } catch (err) {
 535                 throw new ResponseInvalidError("reader error " + err);
 536         }
 537 
 538         var type = reader.peek ();
 539 
 540         if (type == PduType.GetResponse) {
 541                 this.pdu = new GetResponsePdu (reader);
 542         } else {
 543                 throw new ResponseInvalidError ("Unknown PDU type '" + type
 544                                 + "' in response");
 545         }
 546 };

from node-net-snmp.

What fixes it?

from node-net-snmp.

And what is it that is being fixed?

from node-net-snmp.

Well, should it completely shut down if one malformed packet is sent for decoding?

Especially with it being udp.

from node-net-snmp.

Mm, it doesn't completely shutdown, it catches the error in the onMsg() method, and emits an error event.

Could it be that you are not correctly handling the error event from the session, which then causes the Unhandled 'error' event exception ?

from node-net-snmp.

I am using the example exactly as shown in the README.

Look at this:

events.js:136
      throw er; // Unhandled 'error' event
      ^

InvalidAsn1Error: Expected 0x2: got 0x0
    at InvalidAsn1Error (/home/zip/gping/node_modules/asn1-ber/lib/ber/errors.js:4:11)
    at Reader._readTag (/home/zip/gping/node_modules/asn1-ber/lib/ber/reader.js:237:9)
    at Reader.readInt (/home/zip/gping/node_modules/asn1-ber/lib/ber/reader.js:147:14)
    at new ResponseMessage (/home/zip/gping/node_modules/net-snmp/index.js:531:24)
    at Session.onMsg (/home/zip/gping/node_modules/net-snmp/index.js:911:17)
    at Socket.emit (events.js:159:13)
    at UDP.onMessage [as onmessage] (dgram.js:658:8)

All of those errors are from net-snmp or code which net-snmp require()'s.

from node-net-snmp.

Which example specifically?

from node-net-snmp.

This, session.walk as shown in README.md

var oid = "1.3.6.1.2.1.2.2";

function doneCb (error) {
    if (error)
        console.error (error.toString ());
}

function feedCb (varbinds) {
    for (var i = 0; i < varbinds.length; i++) {
        if (snmp.isVarbindError (varbinds[i]))
            console.error (snmp.varbindError (varbinds[i]));
        else
            console.log (varbinds[i].oid + "|" + varbinds[i].value);
    }
}

var maxRepetitions = 20;

// The maxRepetitions argument is optional, and will be ignored unless using
// SNMP verison 2c
session.walk (oid, maxRepetitions, feedCb, doneCb);

from node-net-snmp.

I cannot understand why you would double up on the error handling with isVarbindError and then not include an error handler in the examples.

I mean, why even have a readme at all?

You could just as well make a 'kljadshfl' event and be like, well dude you aren't using the right event!

Why!!!?

from node-net-snmp.

https://stackoverflow.com/questions/5178869/listen-to-all-emitted-events-in-node-js

Is this you? https://en.wikipedia.org/wiki/Steve_Vickers_(computer_scientist)

from node-net-snmp.

Yes, you are right, I mean why even bother releasing it at all - just so ungrateful people like you can criticize it.

If you took some time to read the SNMP RFC's, understood the differences between SNMP v1 and v2c, and understand how it is not possible to attribute unparse-able messages to a specific session ID, and thus callback, then maybe you would have an appreciation as to why it works like it does.

I have spent a considerable amount of my own time documenting this library so I could open-source it and allow others to use it, I don't appreciate comments like this.

If you don't like the library, don't use it, and go and write your own!

from node-net-snmp.

Well,

How else would it get better?

Take for example, https://github.com/iputils/iputils/blob/master/ping.c

Copyright 1989, when's the last time someone criticized it? That is how it got so good as well as easy to use. Also pretty easy to read the source for C, nod.

I am not trying to be an asshole, but at the same time if we are reconstructing the hammer from the rock I want to explain what a handle is and the process of discovering it is refinement, i.e. requesting improvements, i.e. criticizing (cause that's what it becomes when one person does it alone, because everyone else thinks fight to understand rather than work together).

I am positive you know you need the error handler, I don't think people reading the readme do. I think those people would probably try to use it and just give up on it when it's really not supposed to be that hard.

Sorry, but personally I prefer a logged request for improvement that can be re-read and worked on over time rather than an automatic program of "procedural list of problems we had and continue to add to" being shot at me every day.

Why make it personal, just validate the request and fix it or at least document it for someone else to fix.... isn't that the whole idea of open source... or fork/gabeln? Sorry, but if I am working it is about making it work right.

For you it probably boils down to why define each variable type (c vs js), well in English why write your own library... I do know the documentation here is better than my old scanned and copied "The C Programming Language" book. Splicing an array is a hell of a lot faster than counting bytes of memory while free'ing everything else. Tying a knot is a lot faster than splicing a line but you damn sure aren't going to bond it to that cable.

from node-net-snmp.

I really don't understand, is your idea that by not "making it simple" that you would steer an inquisitive mind to reading the RFC in order to fix it?

It's just the strangest book I've ever heard of because you can't flip through it and focus on the names, there's just too many layers.

Are you hoping that once they read the RFC, then setup routing and BGP to discover what an ARP table is that someone will "see" the cable running to their house and cut it so they can really understand what OSI's lowest layer is... does everyone really need to go through that?

from node-net-snmp.

That was my point!

You rely on other people to understand certain things and then to provide their own implementation of them - a benefit of open-source. During their implementation they will have done things in certain ways and it would be hard for someone else to fully appreciate why unless they had traveled the same path.

Your program was throwing an exception because you were not catching error events emitted by a SNMP session. If you cannot parse a response message how can you determine the session ID, determine which request it is associated with, and thus call the requests callback to indicate an error?

How would you suggest you handle such a condition?

I don't understand what you don't think is simple about this library. There is enough documentation, and if you understood SNMP, I believe, you would indeed think it's interface is simple.

I am more than happy to help people out, but I have no time for people to throw mud at me just because something doesn't work how they want.

You are questioning the way I have implemented something, if you think it is not correct please detail why? (emphasis on detail here)

from node-net-snmp.

You could either use an event emitter (as it's not really needed to shut down the entire program when one malformed packet is received, especially for snmp) or you could write in the readme in big bold letters that it's required to capture the errors.

What do you do when you get an error about a malformed packet anyway? There's not much you can do about it and it's quite expected being a protocol stacked on udp.

from node-net-snmp.

I don't think you understand exactly what is going on. The module already uses event emitter:

https://github.com/stephenwvickers/node-net-snmp/blob/master/index.js#L936

Your problem is that you don't catch the error event.

This is the same for every module, if you don't catch the error event, and one is generated by any module, it will bring down your process.

from node-net-snmp.

Well,

const EventEmitter = require('events');

class MyEmitter extends EventEmitter {}

const myEmitter = new MyEmitter();

// THE PROGRAM DOES NOT END EVEN IF THE EVENT IS NOT CAUGHT
//myEmitter.on('error', () => {
//	console.log('an event occurred!');
//});


var loop = function() {

	console.log('loop');
	myEmitter.emit('error');

}

setInterval(loop, 5000);
loop();

It's only required if you use throw, and why would you force the catching of an error in a program which is based on a protocol (UDP) which was built to not worry about every packet getting through.

This isn't a realtime user input expecting a response over the network type of thing, you would use tcp for that like everything that uses tcp.

from node-net-snmp.

The point is simple, the way you documented your required error handling requires that the user handle the error for a protocol that doesn't require the error to be handled.

Are you using Windows and expecting people to not have a console? There's still a standard output there so why force the user to catch an error event and not even tell them it's required in the README?

The honest truth is that people are going to use your example, then it's going to fail on the first malformed packet and the user is going to have no idea why. Entirely due to either forcing a catch of the error handler or not properly explaining it.

from node-net-snmp.

To simplify and clarify.

Let's say that I did capture the error event, what would I do with the error?

• log it to stdout (that does nothing for me)
• retry the request (I could do that without the error event being required but there is no purpose in it for this application)

from node-net-snmp.

Did you actually test your event program? I pasted it into a new file, ran it as is, and got:

PS C:\temp\node-support> node .\error-events.js
loop
events.js:165
      throw err;
      ^

Error: Uncaught, unspecified "error" event. (undefined)
    at MyEmitter.emit (events.js:163:17)
    at loop (C:\temp\node-support\error-events.js:16:12)
    at Object.<anonymous> (C:\temp\node-support\error-events.js:21:1)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)

If I uncomment line 8, 9 and 10 I get:

PS C:\temp\node-support> node .\error-events.js
loop
an event occurred!
loop
an event occurred!

This falls inline with my claim, that you are NOT catching errors from the session.

It's only required if you use throw, and why would you force the catching of an error in a program which is based on a protocol (UDP) which was built to not worry about every packet getting through.

I think your understanding is not correct, catching errors like this is NOT required only for throw, in fact it is completely unrelated to throw.

Additionally, NOT receiving a response to a request and receiving a response that is malformed are NOT the same and require different error handling. Why would you not want to know a device is emitting unexpected results, and be misled that the device is not responding. Only to be surprised when you load Wireshark and see the response - imagine the wild goose chase that would incur.

Think about it, why does ICMP event exist?

The point is simple, the way you documented your required error handling requires that the user handle the error for a protocol that doesn't require the error to be handled.

This claim is simply untrue, any protocol running over UDP will have some form of "error" handling. Even UDP sockets emit errors:

https://nodejs.org/dist/latest-v9.x/docs/api/dgram.html#dgram_event_error

If we didn't catch this error here:

https://github.com/stephenwvickers/node-net-snmp/blob/master/index.js#L593

It too would also bring down your program. I don't see anything in the docs about requiring all users to catch that error event!

Are you using Windows and expecting people to not have a console? There's still a standard output there so why force the user to catch an error event and not even tell them it's required in the README?

I don't know why you would bring this up. The platform in use will have no bearing on this issue.

The honest truth is that people are going to use your example, then it's going to fail on the first malformed packet and the user is going to have no idea why. Entirely due to either forcing a catch of the error handler or not properly explaining it.

Well, the user will at least know it is a malformed packet, and NOT think it's because the network traffic is not reaching their application.

The error event is documented here:

https://www.npmjs.com/package/net-snmp#sessionon-error-callback

It is NOT required to catch a session error to use it to communicate with a functioning device. If an error does occur you will need to design your own strategy for exception handling, and determine if you would like to catch errors and handle them yourself, instead of node handling them and bringing down your program.

This is similar to say try/catch in node, wrap some code in a try block if you don't want it to bring down your program.

Let's say that I did capture the error event, what would I do with the error?

• log it to stdout (that does nothing for me)
• retry the request (I could do that without the error event being required but there is no purpose in it for this application)

Well, most people will have some form of strategy for handling exceptions and errors. In this case, since it's at the session level, I would kill the session and end all communication with the device, and then try to understand why the device is (or is not) sending invalid responses. Retrying the request will simply result in an error being emitted again.

from node-net-snmp.

This application both pings and gets data from snmp for a number of hosts. The reason I don't need to know if a malformed packet was sent via snmp has 2 facets.

First, it would be responding to ping so I would know something is going on with snmp and run a tcpdump at that point.

Second, it would show on the chart exactly when the snmp traffic started failing and if it continued I could tcpdump.

The just of it is, why not either not require the error handler or document it in the readme that it is required?

Oh yea, about the bug in the example of how you don't need to force errors to be handled... thanks for the bug report and here is the solution

var function_to_emit_error = function(cb) {
	// true denotes an error in first argument
	cb(true, 'returned content');
}

var loop = function() {

	function_to_emit_error(function(err, content) {
		// this is not needed, to catch the error and it is quite common in node
		// sorry, I didn't exhaustively test something I am just trying to verify a statement as true with
		// I thought you would already have known that you could have an error sent without it actually being required

		// regarding the packet response not on the same callback, you can use the async library and event emitter inside of
		// a callback in this fashion

		//if (err) {
		//	console.log('error');
		//}

		console.log(content);
	});

}

setInterval(loop, 5000);
loop();

from node-net-snmp.