From e11375a882ff9b54b0ed6fa9ce18c477f37d4fb0 Mon Sep 17 00:00:00 2001 From: DualFroz Date: Fri, 4 Sep 2026 04:36:21 +0200 Subject: [PATCH] fix: allow increment of 1 in Monotonic entropy for io.Reader sources MonotonicEntropy.random rejected a validly drawn value of 0 in its rejection-sampling loop, making an increment of exactly 1 unreachable for entropy sources without a fast Int63n path (e.g. crypto/rand.Reader). For inc == 2 this made the increment fully deterministic (always 2), contradicting the documented "random number between 1 and inc inclusive" contract. Restructure the loop to match the crypto/rand.Int pattern it is adapted from, so 0 is a valid draw and the result is correctly uniform over [1, inc]. --- ulid.go | 14 ++++++++---- ulid_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/ulid.go b/ulid.go index e07e1a6..6d3c6c8 100644 --- a/ulid.go +++ b/ulid.go @@ -641,7 +641,7 @@ func (m *MonotonicEntropy) increment() error { return nil } -// random returns a uniform random value in [1, m.inc), reading entropy +// random returns a uniform random value in [1, m.inc], reading entropy // from m.Reader. When m.inc == 0 || m.inc == 1, it returns 1. // Adapted from: https://golang.org/pkg/crypto/rand/#Int func (m *MonotonicEntropy) random() (inc uint64, err error) { @@ -651,7 +651,7 @@ func (m *MonotonicEntropy) random() (inc uint64, err error) { // Fast path for using a underlying rand.Rand directly. if m.rng != nil { - // Range: [1, m.inc) + // Range: [1, m.inc] return 1 + uint64(m.rng.Int63n(int64(m.inc))), nil } @@ -667,7 +667,7 @@ func (m *MonotonicEntropy) random() (inc uint64, err error) { msbitLen = 8 } - for inc == 0 || inc >= m.inc { + for { if _, err = io.ReadFull(m.Reader, m.rand[:byteLen]); err != nil { return 0, err } @@ -688,9 +688,15 @@ func (m *MonotonicEntropy) random() (inc uint64, err error) { case 5, 6, 7, 8: inc = uint64(binary.LittleEndian.Uint64(m.rand[:8])) } + + // inc is uniform over [0, 2^byteLen*8), which may exceed m.inc; if + // so, redraw to avoid bias toward the low end of [0, m.inc). + if inc < m.inc { + break + } } - // Range: [1, m.inc) + // Range: [1, m.inc] return 1 + inc, nil } diff --git a/ulid_test.go b/ulid_test.go index afa194b..0c83e17 100644 --- a/ulid_test.go +++ b/ulid_test.go @@ -19,6 +19,7 @@ import ( "fmt" "io" "math" + "math/big" "math/rand" "strings" "testing" @@ -629,6 +630,65 @@ func TestMonotonic(t *testing.T) { } } +// TestMonotonicIncrementRange is a regression test for a bug where +// MonotonicEntropy.random incorrectly rejected a masked draw of 0, making an +// increment of 1 unreachable and, for inc == 2, making the increment fully +// deterministic (always 2). It drives Monotonic with a reader that yields an +// exact, controlled byte sequence so the test result does not depend on +// chance draws from a real entropy source. +// +// With inc == 2, random() reads one byte at a time and keeps only its low 2 +// bits (see MonotonicEntropy.random). A masked value of 0 must yield an +// increment of 1, a masked value of 1 must yield an increment of 2, and +// masked values of 2 or 3 are out of range and must be rejected and redrawn. +func TestMonotonicIncrementRange(t *testing.T) { + t.Parallel() + + // The first 10 bytes seed the initial entropy for the first ULID; their + // bit pattern doesn't matter to random(), only that the resulting value + // is non-zero and far from overflow. The remaining bytes are consumed by + // random(), one at a time (or more, on rejection), to compute each + // subsequent increment. + seed := append([]byte{0x01}, bytes.Repeat([]byte{0x00}, 9)...) + draws := []byte{0x00, 0x01, 0x02, 0x03, 0x00} + reader := bytes.NewReader(append(seed, draws...)) + + entropy := ulid.Monotonic(reader, 2) + + prev, err := ulid.New(123, entropy) + if err != nil { + t.Fatal(err) + } + + // draw byte 0x00 -> masked 0 -> accepted -> increment 1 + // draw byte 0x01 -> masked 1 -> accepted -> increment 2 + // draw byte 0x02 -> masked 2 -> rejected (>= inc), redraw + // draw byte 0x03 -> masked 3 -> rejected (>= inc), redraw + // draw byte 0x00 -> masked 0 -> accepted -> increment 1 + wantDeltas := []uint64{1, 2, 1} + + for i, want := range wantDeltas { + next, err := ulid.New(123, entropy) + if err != nil { + t.Fatalf("draw %d: %v", i, err) + } + + if prev.Compare(next) >= 0 { + t.Fatalf("draw %d: monotonicity violated: prev=%v next=%v", i, prev, next) + } + + delta := new(big.Int).Sub( + new(big.Int).SetBytes(next.Entropy()), + new(big.Int).SetBytes(prev.Entropy()), + ).Uint64() + if delta != want { + t.Fatalf("draw %d: delta = %d, want %d", i, delta, want) + } + + prev = next + } +} + func TestMonotonicOverflow(t *testing.T) { t.Parallel()