slayercat / GoSNMPServer

GoSNMPServer is an SNMP server library fully written in Go. It provides Server Get, GetNext, GetBulk, Walk, BulkWalk, Set and Traps. It supports IPv4 and IPv6, using SNMPv2c or SNMPv3.
BSD 2-Clause "Simplified" License
90 stars 51 forks source link

BUG:oidToByteString has #16

Closed ZaneXo closed 1 year ago

ZaneXo commented 1 year ago

Why implement oidToByteString like the following, each time a new request is received, such as Get, Get-Next, it will perform multiple oidToByteString operations, there is additional cpu overhead, and this algorithm still has BUG :

func oidToByteString(oid string) string {
    xi := strings.Split(oid, ".")
    out := []rune{}
    for id, each := range xi {
        if each == "" {
            if id == 0 {
                continue
            } else {
                panic(errors.Errorf("oidToByteString not valid id. value=%v", oid))
            }

        }
        i, err := strconv.ParseInt(each, 10, 32)
        if err != nil {
            panic(err)
        }
        out = append(out, rune(i))
    }
    return string(out)
}

Input oids as:

1.3.6.1.4.1.11475.0.2041601.1.632.0
1.3.6.1.4.1.11475.0.2041602.1.632.0

Error Result as:


> Call API as: 
getForPDUValueControl("1.3.6.1.4.1.11475.0.2041601.1.632.0") 

> Output as:
1.3.6.1.4.1.11475.0.2041602.1.632.0 , 1

Why not return the oid string directly, use the oid string to sort and search the slice,like:

func oidToByteString(oid string) string {
    return oid
}

Will doing this cause any problems? If it is to verify the legitimacy of the OID, is it more appropriate to only perform the verification in SyncConfig, not when subsequent new requests flow in?

ZaneXo commented 1 year ago

@bug:[#17](bug:oidToByteString (#17))bug:oidToByteString (https://github.com/slayercat/GoSNMPServer/pull/17)

provector commented 1 year ago

It does cause problems exactly as stated over the function comment. First, if you try to load any walks from real world and a complicated device during sync function sort triggers the rune bug and you get missing/duplicate oids which prevets server from starting. Second problem occurs during getPDU value function. In sort search when you do comparison on both oids, it triggers the bug again resulting in getnext or walk on client side getting wrong oid order error. I found a working solution for both problems before stumbling on this post, ill test if reverting changes fixes the issue. If not I'll post a PR after some more research and validation.