Problems with function (s sortedErrors) Less"

[jccardonar]

Hello,

I am using goyang to process MIB-YANG modules obtained after converting SMIv2 files with SMIDUMP.

This issue is about special errors that trigger "index out of range" while processing a module.

To reproduce:

• Clone the modules in repository https://github.com/jccardonar/goyang_reports
• Process the module CISCO-IMAGE-TC.yang with goyang:

goyang CISCO-IMAGE-TC.yang

Output should be:

goyang CISCO-IETF-PW-ATM-MIB.yang
panic: runtime error: index out of range
 
goroutine 1 [running]:
github.com/openconfig/goyang/pkg/yang.sortedErrors.Less.func1(0x1, 0x5c)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/entry.go:1058 +0x10d
github.com/openconfig/goyang/pkg/yang.sortedErrors.Less(0xc8203c2fc0, 0x6, 0x6, 0x1, 0x0, 0x422999)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/entry.go:1061 +0x27f
github.com/openconfig/goyang/pkg/yang.(*sortedErrors).Less(0xc8204bf840, 0x1, 0x0, 0x422169)
                <autogenerated>:67 +0xb5
sort.insertionSort(0x7f72c9e86750, 0xc8204bf840, 0x0, 0x6)
                /usr/lib/golang/src/sort/sort.go:32 +0x6f
sort.quickSort(0x7f72c9e86750, 0xc8204bf840, 0x0, 0x6, 0x6)
                /usr/lib/golang/src/sort/sort.go:184 +0x153
sort.Sort(0x7f72c9e86750, 0xc8204bf840)
                /usr/lib/golang/src/sort/sort.go:199 +0x74
github.com/openconfig/goyang/pkg/yang.errorSort(0xc8203a7680, 0x6, 0x8, 0x0, 0x0, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/entry.go:1085 +0x2c6
github.com/openconfig/goyang/pkg/yang.(*Entry).GetErrors(0xc820133c20, 0x0, 0x0, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/entry.go:338 +0xbd
github.com/openconfig/goyang/pkg/yang.(*Modules).Process(0xc8200bc0c0, 0x0, 0x0, 0x0)
                /home/camcardo/go/src/github.com/openconfig/goyang/pkg/yang/modules.go:380 +0xc96
main.main()
                /home/camcardo/go/src/github.com/openconfig/goyang/yang.go:192 +0x18bb 

The function raising the errors is sortedErrors.Less:

func (s sortedErrors) Less(i, j int) bool {
        fi := strings.SplitN(s[i].s, ":", 4)
        fj := strings.SplitN(s[j].s, ":", 4)
        if fi[0] < fj[0] {
                return true
        }
        if fi[0] > fj[0] {
                return false
        }
 
        // compare compares field x to see which is less.
        // numbers are compared as numbers.
        compare := func(x int) int {
                switch {
                case len(fi) == x && len(fj) > x:
                        return -1
                case len(fj) == x && len(fi) > x:
                        return 1
                case len(fj) < x || len(fi) < x:
                        return 0
                }
                return nless(fi[x], fj[x])
        }
        for x := 1; x < 4; x++ {
                switch compare(1) {
                case -1:
                        return true
                case 1:
                        return false
                }
        }
        return false
}

Two observations here:

• I am not sure why the loop between 1 and 4, when the compare function is only being called with 1.
• A new case should be added in the compare function to test for the out of range. Alternative, the case len(fj) < x || len(fi) < x should probably be modified to test that x - 1is not larger than the length of both arrays. Note that x is not a length, but a position in the array (starting with 0).

Thanks.

[andaru]

Thanks, would agree the call compare(1) is in error.

This sort function is designed to sort an incoming string (from some error.Error()) by filename, then line number, column number and finally the error message, so the loop bounds look accurate, because we compared the 0th element (filename) in the first two branches of the function before checking further fields in the case of the first being equal.

However, we can produce error strings which don't have 4 fields; I suspect this type of message was the case here. As strings.SplitN returns a slice with maximum length of its final argument, agree with the bounds checking observation to avoid the panic.