go-restruct / restruct

Rich binary (de)serialization library for Golang
https://restruct.io/
ISC License
357 stars 17 forks source link

Integers not parsed as two's complement if not on byte boundary #46

Closed umeat closed 3 years ago

umeat commented 3 years ago

Integers which don't sit on byte boundaries aren't correctly parsed as two's complement.

If you have a int4 which is "1111" (that's -1 converted), restruct seems to unpack this as "00001111" which is +15 converted.

An example of this in practice can be found here: https://github.com/go-gnss/rtcm/blob/master/rtcm3/antenna.go#L8. The Int64 values in the AntennaReferencePoint type are never correct if the value is supposed to be negative.

If you checkout the code and add the following function to messages_test.go you can see the effect:

func TestCoords(t *testing.T) {
    binary := readPayload(1006)
    msg := rtcm3.DeserializeMessage(binary)
    c := msg.(rtcm3.Message1006)
    t.Error("from restruct", c.ReferencePointX, c.ReferencePointY, c.ReferencePointZ)

    x := (int64(binary[4]&63) << 32) | (int64(binary[5]) << 24) | (int64(binary[6]) << 16) | (int64(binary[7]) << 8) | int64(binary[8])
    if binary[4]&32 == 32 {
        x = x - int64(math.Pow(2, 38))
    }
    y := (int64(binary[9]&63) << 32) | (int64(binary[10]) << 24) | (int64(binary[11]) << 16) | (int64(binary[12]) << 8) | int64(binary[13])
    if binary[9]&32 == 32 {
        y = y - int64(math.Pow(2, 38))
    }
    z := (int64(binary[14]&63) << 32) | (int64(binary[15]) << 24) | (int64(binary[16]) << 16) | (int64(binary[17]) << 8) | int64(binary[18])
    if binary[14]&32 == 32 {
        z = z - int64(math.Pow(2, 38))
    }
    t.Error("bit shifting", x, y, z)
}

This produces the following result:

$ go test .
--- FAIL: TestCoords (0.00s)
    messages_test.go:37: from restruct 230154331576 26704851794 238184162681
    messages_test.go:51: bit shifting -44723575368 26704851794 -36693744263

The values from the bit shifting are correct.

jchv commented 3 years ago

Thanks for the report.

Clearly there needs to be more testing around bitpacking, something that has been a bit neglected up till now. I’ll try to add tests that ensure sign extension occurs correctly with different circumstances when I get a chance.

umeat commented 3 years ago

I haven't looked at how parsing these ints is currently implemented, but after unpacking into the int could do something like (say with int64):

// where bits is the number of bits containing the int and val is the int64 which the bits have been unpacked into
if bits < 64 {
    if val & math.Pow(2, bits-1) == math.Pow(2, bits-1) {
        val = val - math.Pow(2, bits)
    }
}
jchv commented 3 years ago

@umeat I found a couple of bugs with bitpacking and tried to resolve them. Please take a look at PR #47 which I plan on merging as long as it seems to work.