Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions ulid.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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
}

Expand All @@ -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
}
Expand All @@ -688,9 +688,14 @@ func (m *MonotonicEntropy) random() (inc uint64, err error) {
case 5, 6, 7, 8:
inc = uint64(binary.LittleEndian.Uint64(m.rand[:8]))
}

// inc may exceed m.inc; redraw rather than bias the low end of the range.
if inc < m.inc {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This redraw check still samples from a truncated range for multi-byte draws. Line 677 masks m.rand[0], but the bytes are decoded as little-endian below, so the excess-bit mask belongs on m.rand[byteLen-1]. On this PR head, Monotonic(reader, 256) with a nonzero 10-byte seed followed by draw bytes ff 00 yields an entropy increment of 2 across two New(123, entropy) calls; the little-endian candidate is 255, so the increment should be 256. In 200 crypto/rand.Reader draws with this bound I observed only increments 1 or 2. Could you move the mask to the most significant byte and add a regression case across the one-byte boundary?

break
}
}

// Range: [1, m.inc)
// Range: [1, m.inc]
return 1 + inc, nil
}

Expand Down
44 changes: 44 additions & 0 deletions ulid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"io"
"math"
"math/big"
"math/rand"
"strings"
"testing"
Expand Down Expand Up @@ -629,6 +630,49 @@ func TestMonotonic(t *testing.T) {
}
}

// Regression test: random() rejected a masked draw of 0, making an
// increment of 1 unreachable and inc == 2 fully deterministic. The reader
// yields an exact byte sequence so the result does not depend on chance.
func TestMonotonicIncrementRange(t *testing.T) {
t.Parallel()

// The first 10 bytes seed the first ULID; the rest are consumed by random().
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)
}

// masked 0 and 1 are accepted as increments 1 and 2; 2 and 3 are redrawn
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()

Expand Down