Incorrect OID construction for MIBs with standard object name conflicts

[CarlHenrik]

Issue

For private enterprise MIBs that contain object definitions with names that conflict with object definitions in standard MIBs, the OID may sometimes be incorrectly constructed.

Example: If a private enterprise MIB contains the object definition system OBJECT IDENTIFIER ::= { privateMIB 1 }, the name system conflicts with the object definition in RFC1158-MIB: system OBJECT IDENTIFIER ::= { mib-2 1 }.

The function constructing the OID iterates through all loaded modules, and if RFC1158-MIB is checked before the private enterprise MIB, we get a false match and the resulting OID is incorrect. This is likely also the case with any other conflicting object names.

Code

https://github.com/markabrahams/node-net-snmp/blob/fe4fdf6e1f2be0f1c57ba7e3e5fa422cad9cdd81/lib/mib.js#L785-L792

How to reproduce

1. Load a private enterprise MIB with an object definition that conflicts with an object definition from any of the standard MIBs included in the library, such as the example provided with system.
2. Set breakpoints at the for-loop in referenced code and observe the iteration order of modules. Not sure what determines the order here, but if conflicting standard MIB is checked first, the OID will be incorrect.

Proposed solution

To resolve the OID construction issue, I propose that the OID construction process be enhanced by explicitly prioritizing the ModuleName from which the current object definition originates. This can be achieved by ensuring that, when constructing the OID, the system first checks for object definitions within the contextually provided ModuleName before searching through other loaded MIB modules. This approach will prevent false matches by ensuring that the search for object definitions respects the intended module scope, thereby eliminating incorrect OID construction due to object name conflicts.

[markabrahams]

Thanks for logging this @CarlHenrik! That's a good observation. In fact, only the intended module scope should be checked - we shouldn't be looking through other modules.

[CarlHenrik]

For an example of this, you can look at line 139 in the MIB I uploaded previously (NETSURE-MIB-001-B.txt), which fails with the current version due to conflict in RFC1158-MIB.

[CarlHenrik]

@markabrahams, you stated:

In fact, only the intended module scope should be checked - we shouldn't be looking through other modules.

If by "module scope", you also consider the imported modules, then I agree. As the function recursively traverse the parent objects, it will get to a point where it hits an object identifier defined in another module, ideally in one of the imports.

So at least the function should check the current module first, then proceed to check the modules in the IMPORTS property of the current module. Do you agree?

[markabrahams]

If by "module scope", you also consider the imported modules, then I agree. As the function recursively traverse the parent objects, it will get to a point where it hits an object identifier defined in another module, ideally in one of the imports.

There's no recursion or traversal required - SNMP modules IMPORT specific objects FROM other specific modules. So we just need to go and fish the objects directly out of the modules specified in the IMPORT statements.

So at least the function should check the current module first, then proceed to check the modules in the IMPORTS property of the current module. Do you agree?

Yep - that approach is spot on! Specifically, if the named parent isn't found in the current module, the function will check the module that has the IMPORT for the named parent.

[CarlHenrik]

There's no recursion or traversal required - SNMP modules IMPORT specific objects FROM other specific modules. So we just need to go and fish the objects directly out of the modules specified in the IMPORT statements.

By recursive, I was refering to the OID function, which calls itself on line 795: https://github.com/markabrahams/node-net-snmp/blob/b179dd0868692d27df152e32aabeb795accb49cc/lib/mib.js#L735 https://github.com/markabrahams/node-net-snmp/blob/b179dd0868692d27df152e32aabeb795accb49cc/lib/mib.js#L792-L799

Are you looking into refactoring it to not being recursive? I 100% support not using recursive functions, unless abolutely necessary. I often find them a pain to maintain - but that's just my opinion.

[markabrahams]

Are you looking into refactoring it to not being recursive? I 100% support not using recursive functions, unless abolutely necessary. I often find them a pain to maintain - but that's just my opinion.

I tend to agree with your stance on recursion. Recursion has its place, but given the complexity increase in recursive solutions, you'd better have a good reason for it! And yes, that's exactly what I've done - I've removed the recursion and simplified the function to do a local lookup and, failing that, a single lookup in its importing module (if it exists). The former arbitrary module traversal was more a fishing expedition than a good (or correct!) idea...

I've fixed this now, and published the fix in version 3.11.0 of the npm.

[CarlHenrik]

Very nice, works perfectly!