Skip to content

Commit

Permalink
fix: properly encode/decode zero nanoseconds in Duration type
Browse files Browse the repository at this point in the history
Now, zero nanoseconds are properly decoded, and the lexicographical order of the bytes
is correct when comparing two Duration objects: one with negative nanoseconds and another
with zero nanoseconds.

Fixed: cosmos#19910
  • Loading branch information
EmilGeorgiev committed Mar 31, 2024
1 parent cb5d34e commit 100d89d
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 40 deletions.
69 changes: 40 additions & 29 deletions orm/encoding/ormfield/duration.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,32 +37,21 @@ func (d DurationCodec) Encode(value protoreflect.Value, w io.Writer) error {

seconds, nanos := getDurationSecondsAndNanos(value)
secondsInt := seconds.Int()
if secondsInt < DurationSecondsMin || secondsInt > DurationSecondsMax {
return fmt.Errorf("duration seconds is out of range %d, must be between %d and %d", secondsInt, DurationSecondsMin, DurationSecondsMax)
nanosInt := nanos.Int()

if err := validateDurationRanges(secondsInt, nanosInt); err != nil {
return err
}
negative := secondsInt < 0

// we subtract the min duration value to make sure secondsInt is always non-negative and starts at 0.
secondsInt -= DurationSecondsMin
err := encodeSeconds(secondsInt, w)
if err != nil {
return err
}

nanosInt := nanos.Int()
if nanosInt == 0 {
_, err = w.Write(timestampZeroNanosBz)
return err
}

if negative {
if nanosInt < DurationNanosMin || nanosInt > 0 {
return fmt.Errorf("negative duration nanos is out of range %d, must be between %d and %d", nanosInt, DurationNanosMin, 0)
}
nanosInt = DurationNanosMax + nanosInt + 1
} else if nanosInt < 0 || nanosInt > DurationNanosMax {
return fmt.Errorf("duration nanos is out of range %d, must be between %d and %d", nanosInt, 0, DurationNanosMax)
}

// we subtract the min duration value to make sure nanosInt is always non-negative and starts at 0.
nanosInt -= DurationNanosMin
return encodeNanos(nanosInt, w)
}

Expand All @@ -72,26 +61,18 @@ func (d DurationCodec) Decode(r Reader) (protoreflect.Value, error) {
return protoreflect.Value{}, err
}

// we add the min duration value to get back the original value
// we add the min seconds duration value to get back the original value
seconds += DurationSecondsMin

negative := seconds < 0

msg := durationMsgType.New()
msg.Set(durationSecondsField, protoreflect.ValueOfInt64(seconds))

nanos, err := decodeNanos(r)
if err != nil {
return protoreflect.Value{}, err
}

if nanos == 0 {
return protoreflect.ValueOfMessage(msg), nil
}

if negative {
nanos = nanos - DurationNanosMax - 1
}
// we add the min nanos duration value to get back the original value
nanos += DurationNanosMin

msg.Set(durationNanosField, protoreflect.ValueOfInt32(nanos))
return protoreflect.ValueOfMessage(msg), nil
Expand Down Expand Up @@ -141,6 +122,36 @@ func getDurationSecondsAndNanos(value protoreflect.Value) (protoreflect.Value, p
return msg.Get(durationSecondsField), msg.Get(durationNanosField)
}

// validateDurationRanges checks whether seconds and nanoseconds are in valid ranges
// for a protobuf Duration type. It ensures that seconds are within the allowed range
// and, if seconds are zero or negative, verifies that nanoseconds are also within
// the valid range. For negative seconds, nanoseconds must be non-positive.
// Parameters:
// - seconds: The number of seconds component of the duration.
// - nanos: The number of nanoseconds component of the duration.
//
// Returns:
// - error: An error indicating if the duration components are out of range.
func validateDurationRanges(seconds, nanos int64) error {
if seconds < DurationSecondsMin || seconds > DurationSecondsMax {
return fmt.Errorf("duration seconds is out of range %d, must be between %d and %d", seconds, DurationSecondsMin, DurationSecondsMax)
}

if seconds == 0 {
if nanos < DurationNanosMin || nanos > DurationNanosMax {
return fmt.Errorf("duration nanos is out of range %d, must be between %d and %d", nanos, DurationNanosMin, DurationNanosMax)
}
} else if seconds < 0 {
if nanos < DurationNanosMin || nanos > 0 {
return fmt.Errorf("negative duration nanos is out of range %d, must be between %d and %d", nanos, DurationNanosMin, 0)
}
} else if nanos < 0 || nanos > DurationNanosMax {
return fmt.Errorf("duration nanos is out of range %d, must be between %d and %d", nanos, 0, DurationNanosMax)
}

return nil
}

// DurationV0Codec encodes a google.protobuf.Duration value as 12 bytes using
// Int64Codec for seconds followed by Int32Codec for nanos. This allows for
// sorted iteration.
Expand Down
46 changes: 37 additions & 9 deletions orm/encoding/ormfield/duration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestDuration(t *testing.T) {
"no nanos",
100,
0,
6,
9,
},
{
"with nanos",
Expand Down Expand Up @@ -114,14 +114,6 @@ func TestDurationOutOfRange(t *testing.T) {
},
expectErr: "seconds is out of range",
},
{
name: "positive seconds negative nanos",
dur: &durationpb.Duration{
Seconds: 0,
Nanos: -1,
},
expectErr: "nanos is out of range",
},
{
name: "positive seconds nanos too big",
dur: &durationpb.Duration{
Expand Down Expand Up @@ -241,6 +233,42 @@ func TestDurationCompare(t *testing.T) {
},
want: -1,
},
{
name: "negative seconds equal, dur1 nanos zero",
dur1: &durationpb.Duration{
Seconds: -1,
Nanos: 0,
},
dur2: &durationpb.Duration{
Seconds: -1,
Nanos: -1,
},
want: 1,
},
{
name: "negative seconds equal, dur2 nanos zero",
dur1: &durationpb.Duration{
Seconds: -1,
Nanos: -1,
},
dur2: &durationpb.Duration{
Seconds: -1,
Nanos: 0,
},
want: -1,
},
{
name: "seconds equal and dur1 nanos min values",
dur1: &durationpb.Duration{
Seconds: ormfield.DurationSecondsMin,
Nanos: ormfield.DurationNanosMin,
},
dur2: &durationpb.Duration{
Seconds: ormfield.DurationSecondsMin,
Nanos: -1,
},
want: -1,
},
}

for _, tc := range tt {
Expand Down
14 changes: 12 additions & 2 deletions orm/encoding/ormfield/timestamp.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,13 @@ func encodeNanos(nanosInt int64, w io.Writer) error {
nanosBz[i] = byte(nanosInt)
nanosInt >>= 8
}
nanosBz[0] |= 0xC0

// This condition is crucial to ensure the function's correct behavior when dealing with a Timestamp or Duration encoding.
// Specifically, this function is bypassed for Timestamp values when their nanoseconds part is zero.
// In the decodeNanos function, there's a preliminary check for a zero first byte, which represents all values ≤ 16777215 (00000000 11111111 11111111 11111111).
// Without this adjustment (setting the first byte to 0x80 with is 10000000 in binary format), decodeNanos would incorrectly return 0 for any number ≤ 16777215,
// leading to inaccurate decoding of nanoseconds.
nanosBz[0] |= 0x80
_, err := w.Write(nanosBz[:])
return err
}
Expand Down Expand Up @@ -158,7 +164,11 @@ func decodeNanos(r Reader) (int32, error) {
return 0, io.EOF
}

nanos := int32(b0) & 0x3F // clear first two bits
// Clear the first bit, previously set in encodeNanos, to ensure this logic is applied
// and for numbers ≤ 16777215. This adjustment guarantees that we accurately interpret
// the value as intended when encoding smaller numbers.
nanos := int32(b0) & 0x7F

for i := 0; i < 3; i++ {
nanos <<= 8
nanos |= int32(nanosBz[i])
Expand Down

0 comments on commit 100d89d

Please sign in to comment.