From 100d89df186486a4d714b43d06fe26f4eb5e64d4 Mon Sep 17 00:00:00 2001 From: Emil Georgiev Date: Sun, 31 Mar 2024 15:18:42 +0300 Subject: [PATCH] fix: properly encode/decode zero nanoseconds in Duration type 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: #19910 --- orm/encoding/ormfield/duration.go | 69 +++++++++++++++----------- orm/encoding/ormfield/duration_test.go | 46 +++++++++++++---- orm/encoding/ormfield/timestamp.go | 14 +++++- 3 files changed, 89 insertions(+), 40 deletions(-) diff --git a/orm/encoding/ormfield/duration.go b/orm/encoding/ormfield/duration.go index 078d0ca614775..45b1e4cc94a42 100644 --- a/orm/encoding/ormfield/duration.go +++ b/orm/encoding/ormfield/duration.go @@ -37,10 +37,12 @@ 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) @@ -48,21 +50,8 @@ func (d DurationCodec) Encode(value protoreflect.Value, w io.Writer) error { 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) } @@ -72,11 +61,9 @@ 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)) @@ -84,14 +71,8 @@ func (d DurationCodec) Decode(r Reader) (protoreflect.Value, error) { 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 @@ -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. diff --git a/orm/encoding/ormfield/duration_test.go b/orm/encoding/ormfield/duration_test.go index 474f5db52c7c6..485605ee0c1ba 100644 --- a/orm/encoding/ormfield/duration_test.go +++ b/orm/encoding/ormfield/duration_test.go @@ -38,7 +38,7 @@ func TestDuration(t *testing.T) { "no nanos", 100, 0, - 6, + 9, }, { "with nanos", @@ -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{ @@ -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 { diff --git a/orm/encoding/ormfield/timestamp.go b/orm/encoding/ormfield/timestamp.go index 0049b13f156a8..2e0e459d22dd0 100644 --- a/orm/encoding/ormfield/timestamp.go +++ b/orm/encoding/ormfield/timestamp.go @@ -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 } @@ -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])