[SNMP4J] Counting Invalid Traps

[Frank Fock]

Elise,

Where are the code locations where you think errors
are counted twice?

Do you have a RFC pointer that notes that a BER encoding
error must not counted as an invalid message?

Thanks,
Frank

Elise Atkins wrote:
> 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
>>>
> 
> 

-- 
AGENT++
http://www.agentpp.com
http://www.snmp4j.com
http://www.mibexplorer.com
http://www.mibdesigner.com