Skip to content
Merged
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
8 changes: 4 additions & 4 deletions stream_writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -754,16 +754,16 @@ func TestStreamWriterIncremental(t *testing.T) {
require.NoError(t, sw.Write(buf), "sw.Write() failed")
require.NoError(t, sw.Flush(), "sw.Flush() failed")

buf = z.NewBuffer(10<<20, "test")
defer func() { require.NoError(t, buf.Release()) }()
buf2 := z.NewBuffer(10<<20, "test")
defer func() { require.NoError(t, buf2.Release()) }()
KVToBuffer(&pb.KV{
Key: []byte("a2"),
Value: []byte("val2"),
Version: 9,
}, buf)
}, buf2)
sw = db.NewStreamWriter()
require.NoError(t, sw.PrepareIncremental(), "sw.PrepareIncremental() failed")
require.NoError(t, sw.Write(buf), "sw.Write() failed")
require.NoError(t, sw.Write(buf2), "sw.Write() failed")
require.NoError(t, sw.Flush(), "sw.Flush() failed")

// This will move the maxTs to 10 (earlier, without the fix)
Expand Down
4 changes: 2 additions & 2 deletions test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,6 @@ write_coverage() {
# parallel tests currently not working
# parallel --halt now,fail=1 --progress --line-buffer ::: stream manual root
# run tests in sequence
#root
#stream
root
Copy link
Collaborator

Choose a reason for hiding this comment

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

These mistakenly got commented out in a prior merge.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These mistakenly got commented out in a prior merge.

Thanks for catching the edge case with Entry.Key and the quick merge! Happy to contribute.

stream
manual
8 changes: 6 additions & 2 deletions y/y.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,11 @@ func OpenTruncFile(filename string, sync bool) (*os.File, error) {

// SafeCopy does append(a[:0], src...).
func SafeCopy(a, src []byte) []byte {
return append(a[:0], src...)
b := append(a[:0], src...)
if b == nil {
return []byte{}
}
return b
}

// Copy copies a byte slice and returns the copied slice.
Expand Down Expand Up @@ -134,7 +138,7 @@ func CompareKeys(key1, key2 []byte) int {

// ParseKey parses the actual key from the key bytes.
func ParseKey(key []byte) []byte {
if key == nil {
if len(key) < 8 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@kooltuoehias I added this sanity check here. In practice, keys processed anywhere in badger will always have byte data, but because SafeCopy does now produce zero length slices and Entry.Key values are sent to that function in publisher.go, better safe than sorry. This check mimics ParseTs's logic to prevent panics by ensuring the slice has the capacity that it's operating on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kooltuoehias I added this sanity check here. In practice, keys processed anywhere in badger will always have byte data, but because SafeCopy does now produce zero length slices and Entry.Key values are sent to that function in publisher.go, better safe than sorry. This check mimics ParseTs's logic to prevent panics by ensuring the slice has the capacity that it's operating on.

Many thanks ! Things goes really quick here !

return nil
}

Expand Down
44 changes: 44 additions & 0 deletions y/y_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,3 +328,47 @@ func TestAllocatorReuse(t *testing.T) {
}
t.Logf("Allocator: %s\n", a)
}

func TestSafeCopy_Issue2067(t *testing.T) {
type args struct {
a []byte
src []byte
}
tests := []struct {
name string
args args
want []byte
}{
{
name: "Nil src should return empty slice not nil",
args: args{a: nil, src: nil},
want: []byte{},
},
{
name: "Empty src should return empty slice not nil",
args: args{a: nil, src: []byte{}},
want: []byte{},
},
{
name: "Normal src should return src content",
args: args{a: nil, src: []byte("hello")},
want: []byte("hello"),
},
{
name: "Buffer reuse with nil src should return empty slice",
args: args{a: make([]byte, 10), src: nil},
want: []byte{},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := SafeCopy(tt.args.a, tt.args.src)
require.Equal(t, tt.want, got)

// Explicit check for nil vs empty slice distinction
if len(tt.want) == 0 {
require.NotNil(t, got, "SafeCopy returned nil, but we expected an empty slice []byte{}")
}
})
}
}