crabmusket / gosunspec

SunSpec domain model and use cases for Go. UNMAINTAINED
BSD 3-Clause "New" or "Revised" License
2 stars 6 forks source link

slice bounds out of range panic when unmarshalling common block #32

Closed andig closed 5 years ago

andig commented 5 years ago

Only today I've stumbled on this excellent codebase after working on mbmd for quite some time. I really love it!

However I've stumbled across a problem. This happens when parsing the address space of a grid inverter like this:

in.Do(func(d sunspec.Device) {
    d.Do(func(m sunspec.Model) {
        log.Println(m.Id())

        m.Do(func(b sunspec.Block) {
            err = b.Read()
            if err != nil {
                log.Fatal(err)
            }
            log.Println(b)
        })
    })
})
panic: runtime error: slice bounds out of range

goroutine 1 [running]:
github.com/crabmusket/gosunspec/impl.(*point).Unmarshal(0xc0000b0280, 0xc00020a050, 0x10, 0x34, 0xc00020812e, 0x0)
        /Users/andig/htdocs/go/pkg/mod/github.com/crabmusket/gosunspec@v0.0.0-20170310004250-4c2adf6161ca/impl/point.go:496 +0x12e1
github.com/crabmusket/gosunspec/modbus.(*modbusDriver).Read(0xc0000ae6a0, 0x11b9fc0, 0xc0001a8200, 0x0, 0x0, 0x0, 0x0, 0xc000042d88)
        /Users/andig/htdocs/go/pkg/mod/github.com/crabmusket/gosunspec@v0.0.0-20170310004250-4c2adf6161ca/modbus/modbus.go:156 +0x656
github.com/crabmusket/gosunspec/impl.(*block).Read(0xc0001a8200, 0x0, 0x0, 0x0, 0x13b2d98, 0x0)
        /Users/andig/htdocs/go/pkg/mod/github.com/crabmusket/gosunspec@v0.0.0-20170310004250-4c2adf6161ca/impl/block.go:54 +0x63
main.main.func1.1.1(0x11b8120, 0xc0001a8200)
        /Users/andig/htdocs/mbmd/cmd/scan/main.go:90 +0x4f

This is inside https://github.com/crabmusket/gosunspec/blob/master/impl/point.go#L487, the panic happens right at the first Common block.

I've added some logging to Unmarshal to see what actually goes in (would be nice if a block allowed to "dump" its contents to simplify this):

2019/06/12 20:52:18 {XMLName:{Space: Local:} Id:Mn Offset:0 Length:16 Type:string ScaleFactor: Units: Mandatory:true Access: Symbols:[]}
2019/06/12 20:52:18 Unmarshal: 53 6f 6c 61 72 45 64 67 65 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2019/06/12 20:52:18 {XMLName:{Space: Local:} Id:Md Offset:16 Length:16 Type:string ScaleFactor: Units: Mandatory:true Access: Symbols:[]}
2019/06/12 20:52:18 Unmarshal: 53 45 33 30 30 30 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2019/06/12 20:52:18 {XMLName:{Space: Local:} Id:Opt Offset:32 Length:8 Type:string ScaleFactor: Units: Mandatory:false Access: Symbols:[]}
2019/06/12 20:52:18 Unmarshal: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2019/06/12 20:52:18 {XMLName:{Space: Local:} Id:Vr Offset:40 Length:8 Type:string ScaleFactor: Units: Mandatory:false Access: Symbols:[]}
2019/06/12 20:52:18 Unmarshal: 30 30 30 33 2e 32 31 38 36 00 00 00 00 00 00 00

I think b should actually be copied from bytes which is longer than b as b is only initialized with the length of the point and will fail if the string uses all characters of the point:

            b = bytes[0:i]
andig commented 5 years ago

Mhhm, actually wrong assessment. It seems that Vr should only be 8 bytes but the buffer contains 9. Seems that's what the device sends or the bytes buffer is reused- didn’t check.

So rather loop across b which has correct length to ignore trailing 9nth character?