amazon-ion / ion-go

A Go implementation of Amazon Ion.
https://amazon-ion.github.io/ion-docs/
Apache License 2.0
175 stars 31 forks source link

Timestamp fractional seconds cannot have positive exponents #180

Closed zslayton closed 3 years ago

zslayton commented 3 years ago

PR #177 introduced a subtle bug in the binary reader.

Each Timestamp has a decimal field representing its number of fractional seconds. After reading the decimal's exponent field from the input stream, the reader then casts it to an unsigned int.

func (b *bitstream) readNsecs(length uint64) (int, bool, uint8, error) {
    d, err := b.readDecimal(length)
    if err != nil {
        return 0, false, 0, err
    }

    nsec, err := d.ShiftL(9).trunc()
    if err != nil || nsec < 0 || nsec > 999999999 {
        msg := fmt.Sprintf("invalid timestamp fraction: %v", d)
        return 0, false, 0, &SyntaxError{msg, b.pos}
    }

    nsec, err = d.ShiftL(9).round()
    if err != nil {
        msg := fmt.Sprintf("invalid timestamp fraction: %v", d)
        return 0, false, 0, &SyntaxError{msg, b.pos}
    }

    exponent := uint8(d.scale) // <-------------- HERE 

    // Overflow to second.
    if nsec == 1000000000 {
        return 0, true, exponent, nil
    }

    return int(nsec), false, exponent, nil
}

In most cases this behavior is fine. A decimal's scale field is the negation of the exponent. We don't expect a negative scale; that would indicate a fractional seconds value greater than 1 and therefore represent some number of whole seconds.

However, there's an exception to this: a fractional seconds of 0d1 or any other zero-coefficient, positive-exponent decimal value. In this case, scale is -1 and casting it to a uint8 turns it into the value 255.

Downstream, the constructor NewTimestampWithFractionalSeconds detects that 255 is greater than the maximum supported fractional seconds precision and trims it to 9:

https://github.com/amzn/ion-go/blob/7d02b52c1f33e23baf05d066dcbef6556343989a/ion/timestamp.go#L157-L161

The end result is a Timestamp value with a (correct) coefficient of zero but an erroneously set precision of 9.

cc @byronlin13 @justing-bq