[SNMP4J] Counting Invalid Traps

[Elise Atkins]

Frank,

Thanks for making the change so quickly. I have one nit picky comment. 
If you have a ASN.1 parse error, it gets counted as an error and then 
processing continues and the MPv1 class throws an exception and it 
counted again as an invalid packet. A cleaner solution to prevent double 
counting would be to add a return after the first counter event is fired 
or  log it and not count it and continue processing and have the 
catch(IOException) block count it.

Thanks,
Elise

Frank Fock wrote:
> Hi Elise,
>
> Thank you for reporting this bug [SJF-18].
> The current snapshot contains a fix for it,
> which is slightly different to yours.
>
> Regards,
> Frank
>
> Elise Atkins wrote:
>> I have used SNMP4J for several years and have been very satisfied. 
>> Recently I did some testing to see what happens when invalid V1 traps 
>> and snmp requests were received by the snmp4j code. A CounterListner 
>> was added to the MessageDispatcherImpl and the number of incoming 
>> traps/requests were accurately counted but the number of invalid 
>> traps was not. The large majority of traps were silently discarded. 
>> After looking at the following code snippet from 
>> MessageDispatcherImpl and the MPv1.java code I concluded that when 
>> the MPv1 encountered an error it threw an exception that was caught 
>> by the catch block near the end of the snippet. The packet is then 
>> silently discarded unless the Snmp4JSettings.forwardRuntimeExceptions 
>> is true and the TransportMapping  class catches and counts the 
>> exception.
>>
>> I think that a better solution would be to add the following lines 
>> the catch block:
>>         CounterEvent event = new CounterEvent(this,
>>                                               
>> SnmpConstants.snmpInvalidMsgs);
>>         fireIncrementCounter(event);
>>
>> The packet would be then counted as invalid. I did set the 
>> forwardRuntimeExceptions to true and was able to get a count of the 
>> invalid packets and all seemed ok until a little later. I also use 
>> AgentXSubagent to connect to the snmpd service. When that service was 
>> not running or misconfigured and the subagent could not connect, a 
>> socket was orphaned and eventually things stopped working because 
>> there were too many sockets opened. I fixed that by calling close and 
>> disconnect on the subagent when the connect method failed. But I am 
>> now worried that having the forwardRuntimeExceptions set to true will 
>> cause additional problems down the line.
>>
>>   public void processMessage(TransportMapping sourceTransport,
>>                              Address incomingAddress,
>>                              BERInputStream wholeMessage) {
>>     fireIncrementCounter(new CounterEvent(this, 
>> SnmpConstants.snmpInPkts));
>>     if (!wholeMessage.markSupported()) {
>>       String txt = "Message stream must support marks";
>>       logger.error(txt);
>>       throw new IllegalArgumentException(txt);
>>     }
>>     try {
>>       wholeMessage.mark(16);
>>       BER.MutableByte type = new BER.MutableByte();
>>       // decode header but do not check length here, because we do 
>> only decode
>>       // the first 16 bytes.
>>       BER.decodeHeader(wholeMessage, type, false);
>>       if (type.getValue() != BER.SEQUENCE) {
>>         logger.error("ASN.1 parse error (message is not a sequence)");
>>         CounterEvent event = new CounterEvent(this,
>>                                               
>> SnmpConstants.snmpInASNParseErrs);
>>         fireIncrementCounter(event);
>>       }
>>       Integer32 version = new Integer32();
>>       version.decodeBER(wholeMessage);
>>       MessageProcessingModel mp = 
>> getMessageProcessingModel(version.getValue());
>>       if (mp == null) {
>>         logger.warn("SNMP version "+version+" is not supported");
>>         CounterEvent event = new CounterEvent(this,
>>                                               
>> SnmpConstants.snmpInBadVersions);
>>         fireIncrementCounter(event);
>>       }
>>       else {
>>         // reset it
>>         wholeMessage.reset();
>>         // dispatch it
>>         dispatchMessage(sourceTransport, mp, incomingAddress, 
>> wholeMessage);
>>       }
>>     }
>>     catch (Exception ex) {
>>       logger.error(ex);
>>       if (logger.isDebugEnabled()) {
>>         ex.printStackTrace();
>>       }
>>       if (SNMP4JSettings.isFowardRuntimeExceptions()) {
>>         throw new RuntimeException(ex);
>>       }
>>     }
>>     catch (OutOfMemoryError oex) {
>>       logger.error(oex);
>>       if (SNMP4JSettings.isFowardRuntimeExceptions()) {
>>         throw oex;
>>       }
>>     }
>>   }
>>
>> Sincerely,
>> Elise Atkins
>>
>


-- 
ZoneRanger "Management Through Firewalls" <http://www.tavve.com/?leadlander>

-- 
ZoneRanger "Management Through Firewalls" <http://www.tavve.com/?leadlander>