lunixbochs / struc

Better binary packing for Go
MIT License
564 stars 42 forks source link

sizeof fails for types other than int #21

Closed jchv closed 8 years ago

jchv commented 9 years ago

I'm writing a parser for a specific file type (the .MS3D file format to be exact) and some of the struct sizes are represented by 16-bit unsigned integers. However, a structure like this:

struct {
    ...
    NumVertices  uint16 `struc:"little,sizeof=Vertices"`
    Vertices     []Vertex
    ...
}

...causes a panic like this:

panic: reflect: call of reflect.Value.Int on uint16 Value

goroutine 1 [running, locked to thread]:
reflect.Value.Int(0x534e70, 0xc082056294, 0xc9, 0xc082002520)
        c:/go/src/reflect/value.go:884 +0xad
github.com/lunixbochs/struc.Fields.Unpack(0xc082060000, 0x18, 0x18, 0x7242c0, 0xc082024090, 0x593770, 0xc082056280, 0xd9, 0x0, 0x0)
        C:/Users/johnc/Go/src/github.com/lunixbochs/struc/fields.go:83 +0x269
github.com/lunixbochs/struc.UnpackWithOrder(0x7242c0, 0xc082024090, 0x525710, 0xc082056280, 0x0, 0x0, 0x0, 0x0)
        C:/Users/johnc/Go/src/github.com/lunixbochs/struc/struc.go:64 +0x172
github.com/lunixbochs/struc.Unpack(0x7242c0, 0xc082024090, 0x525710, 0xc082056280, 0x0, 0x0)
        C:/Users/johnc/Go/src/github.com/lunixbochs/struc/struc.go:53 +0x5e
github.com/johnwchadwick/glfwtest/ms3d.LoadModel(0x7242c0, 0xc082024090, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
        C:/Users/johnc/Go/src/github.com/johnwchadwick/glfwtest/ms3d/format.go:120 +0xa6
main.main()
        C:/Users/johnc/Go/src/github.com/johnwchadwick/glfwtest/main.go:20 +0xa1

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
        c:/go/src/runtime/asm_amd64.s:1696 +0x1

It seems to be fixed if I do it this way:

struct {
    ...
    NumVertices  int `struc:"uint16,little,sizeof=Vertices"`
    Vertices     []Vertex
    ...
}

...but I'm currently running into another (seemingly unrelated) panic, so I'm not entirely sure it works how I expect it to.

lunixbochs commented 9 years ago

https://golang.org/pkg/reflect/#Value.Int

Int returns v's underlying value, as an int64.
It panics if v's Kind is not Int, Int8, Int16, Int32, or Int64.

Just need to tweak the Sizeof code here and here to support Uint() as well. Otherwise you can safely store it as an int with a small per-object memory cost.

dmitshur commented 8 years ago

I tried this library, ran into this issue, made a reproducible sample and came here to report it. Found it's already reported. Nothing more I can add here, just +1.

dmitshur commented 8 years ago

Actually, my panic seems slightly different. I'll post my reproducible sample since I've already created it, in case it's helpful:

package main

import (
    "bytes"
    "encoding/binary"
    "fmt"

    "github.com/lunixbochs/struc"
)

var p struct {
    NameLength uint8 `struc:"sizeof=Name"`
    Name       []byte
}

func main() {
    const name = "example"
    p.NameLength = uint8(len(name))
    p.Name = []byte(name)

    fmt.Printf("%#v\n", a())
    fmt.Printf("%#v\n", b())
}

func a() []byte {
    var (
        buf bytes.Buffer
        err error
    )
    err = binary.Write(&buf, binary.BigEndian, &p.NameLength)
    if err != nil {
        panic(err)
    }
    err = binary.Write(&buf, binary.BigEndian, &p.Name)
    if err != nil {
        panic(err)
    }
    return buf.Bytes()
}

func b() []byte {
    var buf bytes.Buffer
    err := struc.Pack(&buf, &p)
    if err != nil {
        panic(err)
    }
    return buf.Bytes()
}

Causes panic in field.go:94:

[]byte{0x7, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65}
panic: reflect: call of reflect.Value.Uint on int Value

goroutine 1 [running]:
reflect.Value.Uint(0x1084a0, 0xc82000a510, 0x42, 0x275000)
    /usr/local/go/src/reflect/value.go:1711 +0xbd
github.com/lunixbochs/struc.(*Field).packVal(0xc820078280, 0xc82000a500, 0x8, 0x8, 0x1084a0, 0xc82000a510, 0x42, 0x1, 0x1, 0x0, ...)
    src/github.com/lunixbochs/struc/field.go:94 +0x31f
github.com/lunixbochs/struc.(*Field).Pack(0xc820078280, 0xc82000a500, 0x8, 0x8, 0x1084a0, 0xc82000a510, 0x42, 0x1, 0xd7, 0x0, ...)
    src/github.com/lunixbochs/struc/field.go:159 +0x392
github.com/lunixbochs/struc.Fields.Pack(0xc82000a470, 0x2, 0x2, 0xc82000a500, 0x8, 0x8, 0x139860, 0x224720, 0xd9, 0x8, ...)
    src/github.com/lunixbochs/struc/fields.go:61 +0x43c
github.com/lunixbochs/struc.(*Fields).Pack(0xc820012200, 0xc82000a500, 0x8, 0x8, 0x139860, 0x224720, 0xd9, 0x0, 0x0, 0x0)
    <autogenerated>:6 +0xfb
github.com/lunixbochs/struc.PackWithOrder(0x4291f8, 0xc8200568c0, 0xfe9c0, 0x224720, 0x0, 0x0, 0x0, 0x0)
    src/github.com/lunixbochs/struc/struc.go:50 +0x398
github.com/lunixbochs/struc.Pack(0x4291f8, 0xc8200568c0, 0xfe9c0, 0x224720, 0x0, 0x0)
    src/github.com/lunixbochs/struc/struc.go:33 +0x57
main.b(0x0, 0x0, 0x0)
    src/github.com/shurcooL/play/31/main.go:43 +0xab
main.main()
    src/github.com/shurcooL/play/31/main.go:22 +0x183
lunixbochs commented 8 years ago

Should be fixed by b5c442e885e11f5a5c995ba23fd57e12777bce69

dmitshur commented 8 years ago

Confirmed fixed. Thanks.

lunixbochs commented 8 years ago

Thanks!