Skip to content

Commit

Permalink
Fix two bugs with unsafe memory access (#253)
Browse files Browse the repository at this point in the history 
* Add unit tests to demonstrate two unsafe bugs

* Fix bug in embedded pointer nil-check in encoder/decoder

Both encoder and decoder examine the wrong variable when checking for nil
embedded pointer in a struct.

* Fix wrong-size allocation of embedded pointer structs in decoder

When a nil embedded pointer is encountered in the decoder, it attempts to
allocate a fresh struct on the heap and assign its address into the embedded
pointer, so nested decoding can continue. However, it mistakenly allocates a
pointer-sized memory block, rather than struct-sized.
  • Loading branch information
@daboyuka
daboyuka authored Apr 22, 2023
1 parent c164ad4 commit f8364c2
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 24 deletions.
6 changes: 3 additions & 3 deletions codec_record.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ func (d *structDecoder) Decode(ptr unsafe.Pointer, r *Reader) {
}

if f.Type().Kind() == reflect.Ptr {
if *((*unsafe.Pointer)(ptr)) == nil {
newPtr := f.Type().UnsafeNew()
if *((*unsafe.Pointer)(fieldPtr)) == nil {
newPtr := f.Type().(*reflect2.UnsafePtrType).Elem().UnsafeNew()
*((*unsafe.Pointer)(fieldPtr)) = newPtr
}

Expand Down Expand Up @@ -208,7 +208,7 @@ func (e *structEncoder) Encode(ptr unsafe.Pointer, w *Writer) {
}

if f.Type().Kind() == reflect.Ptr {
if *((*unsafe.Pointer)(ptr)) == nil {
if *((*unsafe.Pointer)(fieldPtr)) == nil {
w.Error = fmt.Errorf("embedded field %q is nil", f.Name())
return
}
Expand Down
61 changes: 55 additions & 6 deletions decoder_record_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,14 @@ func TestDecoder_RecordStructInvalidData(t *testing.T) {
func TestDecoder_RecordEmbeddedStruct(t *testing.T) {
defer ConfigTeardown()

data := []byte{0x36, 0x06, 0x66, 0x6f, 0x6f}
data := []byte{0x36, 0x06, 0x66, 0x6f, 0x6f, 0x06, 0x62, 0x61, 0x72}
schema := `{
"type": "record",
"name": "test",
"fields" : [
{"name": "a", "type": "long"},
{"name": "b", "type": "string"}
{"name": "b", "type": "string"},
{"name": "c", "type": "string"}
]
}`
dec, err := avro.NewDecoder(schema, bytes.NewReader(data))
Expand All @@ -159,19 +160,20 @@ func TestDecoder_RecordEmbeddedStruct(t *testing.T) {
err = dec.Decode(&got)

require.NoError(t, err)
assert.Equal(t, TestEmbeddedRecord{TestEmbed: TestEmbed{A: 27}, B: "foo"}, got)
assert.Equal(t, TestEmbeddedRecord{TestEmbed: TestEmbed{A: 27, B: "foo"}, C: "bar"}, got)
}

func TestDecoder_RecordEmbeddedPtrStruct(t *testing.T) {
defer ConfigTeardown()

data := []byte{0x36, 0x06, 0x66, 0x6f, 0x6f}
data := []byte{0x36, 0x06, 0x66, 0x6f, 0x6f, 0x06, 0x62, 0x61, 0x72}
schema := `{
"type": "record",
"name": "test",
"fields" : [
{"name": "a", "type": "long"},
{"name": "b", "type": "string"}
{"name": "b", "type": "string"},
{"name": "c", "type": "string"}
]
}`
dec, err := avro.NewDecoder(schema, bytes.NewReader(data))
Expand All @@ -181,7 +183,54 @@ func TestDecoder_RecordEmbeddedPtrStruct(t *testing.T) {
err = dec.Decode(&got)

require.NoError(t, err)
assert.Equal(t, TestEmbeddedPtrRecord{TestEmbed: &TestEmbed{A: 27}, B: "foo"}, got)
assert.Equal(t, TestEmbeddedPtrRecord{TestEmbed: &TestEmbed{A: 27, B: "foo"}, C: "bar"}, got)
}

func TestDecoder_RecordEmbeddedPtrStructNew(t *testing.T) {
defer ConfigTeardown()

data := []byte{0x36, 0x06, 0x66, 0x6f, 0x6f, 0x06, 0x62, 0x61, 0x72}
schema := `{
"type": "record",
"name": "test",
"fields" : [
{"name": "a", "type": "long"},
{"name": "b", "type": "string"},
{"name": "c", "type": "string"}
]
}`
dec, err := avro.NewDecoder(schema, bytes.NewReader(data))
require.NoError(t, err)

var got TestEmbeddedPtrRecord
got.C = "nonzero" // non-zero value here triggers bug in allocating TestEmbed
err = dec.Decode(&got)

require.NoError(t, err)
assert.Equal(t, TestEmbeddedPtrRecord{TestEmbed: &TestEmbed{A: 27, B: "foo"}, C: "bar"}, got)
}

func TestDecoder_RecordEmbeddedIntStruct(t *testing.T) {
defer ConfigTeardown()

data := []byte{0x36, 0x06, 0x66, 0x6f, 0x6f, 0x06, 0x62, 0x61, 0x72}
schema := `{
"type": "record",
"name": "test",
"fields" : [
{"name": "a", "type": "long"},
{"name": "b", "type": "string"},
{"name": "c", "type": "string"}
]
}`
dec, err := avro.NewDecoder(schema, bytes.NewReader(data))
require.NoError(t, err)

var got TestEmbeddedIntRecord
err = dec.Decode(&got)

require.NoError(t, err)
assert.Equal(t, TestEmbeddedIntRecord{B: "foo"}, got)
}

func TestDecoder_RecordMap(t *testing.T) {
Expand Down
19 changes: 11 additions & 8 deletions encoder_record_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,18 +190,19 @@ func TestEncoder_RecordEmbeddedStruct(t *testing.T) {
"name": "test",
"fields" : [
{"name": "a", "type": "long"},
{"name": "b", "type": "string"}
{"name": "b", "type": "string"},
{"name": "c", "type": "string"}
]
}`
obj := TestEmbeddedRecord{TestEmbed: TestEmbed{A: 27}, B: "foo"}
obj := TestEmbeddedRecord{TestEmbed: TestEmbed{A: 27, B: "foo"}, C: "bar"}
buf := &bytes.Buffer{}
enc, err := avro.NewEncoder(schema, buf)
require.NoError(t, err)

err = enc.Encode(obj)

require.NoError(t, err)
assert.Equal(t, []byte{0x36, 0x06, 0x66, 0x6f, 0x6f}, buf.Bytes())
assert.Equal(t, []byte{0x36, 0x06, 0x66, 0x6f, 0x6f, 0x06, 0x62, 0x61, 0x72}, buf.Bytes())
}

func TestEncoder_RecordEmbeddedPtrStruct(t *testing.T) {
Expand All @@ -212,18 +213,19 @@ func TestEncoder_RecordEmbeddedPtrStruct(t *testing.T) {
"name": "test",
"fields" : [
{"name": "a", "type": "long"},
{"name": "b", "type": "string"}
{"name": "b", "type": "string"},
{"name": "c", "type": "string"}
]
}`
obj := TestEmbeddedPtrRecord{TestEmbed: &TestEmbed{A: 27}, B: "foo"}
obj := TestEmbeddedPtrRecord{TestEmbed: &TestEmbed{A: 27, B: "foo"}, C: "bar"}
buf := &bytes.Buffer{}
enc, err := avro.NewEncoder(schema, buf)
require.NoError(t, err)

err = enc.Encode(obj)

require.NoError(t, err)
assert.Equal(t, []byte{0x36, 0x06, 0x66, 0x6f, 0x6f}, buf.Bytes())
assert.Equal(t, []byte{0x36, 0x06, 0x66, 0x6f, 0x6f, 0x06, 0x62, 0x61, 0x72}, buf.Bytes())
}

func TestEncoder_RecordEmbeddedPtrStructNull(t *testing.T) {
Expand All @@ -234,10 +236,11 @@ func TestEncoder_RecordEmbeddedPtrStructNull(t *testing.T) {
"name": "test",
"fields" : [
{"name": "a", "type": "long"},
{"name": "b", "type": "string"}
{"name": "b", "type": "string"},
{"name": "c", "type": "string"}
]
}`
obj := TestEmbeddedPtrRecord{B: "foo"}
obj := TestEmbeddedPtrRecord{C: "foo"}
buf := &bytes.Buffer{}
enc, err := avro.NewEncoder(schema, buf)
require.NoError(t, err)
Expand Down
15 changes: 8 additions & 7 deletions types_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,27 +27,28 @@ type TestUnion struct {
}

type TestEmbeddedRecord struct {
TestEmbed
C string `avro:"c"`

B string `avro:"b"`
TestEmbed // tests not-first position
}

type TestEmbeddedPtrRecord struct {
*TestEmbed
C string `avro:"c"`

B string `avro:"b"`
*TestEmbed // tests not-first position
}

type TestEmbed struct {
A int64 `avro:"a"`
A int64 `avro:"a"`
B string `avro:"b"`
}

type TestEmbedInt int

type TestEmbeddedIntRecord struct {
TestEmbedInt

B string `avro:"b"`

TestEmbedInt // tests not-first position
}

type TestUnexportedRecord struct {
Expand Down

0 comments on commit f8364c2

Please sign in to comment.