Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Timestamp fractional seconds cannot have positive exponents #180

Closed
zslayton opened this issue Jun 10, 2021 · 0 comments
Closed

Timestamp fractional seconds cannot have positive exponents #180

zslayton opened this issue Jun 10, 2021 · 0 comments
Labels
bug Something isn't working

Comments

@zslayton
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant