From f8364c2d56084292b3ad22c2127659e777aa391f Mon Sep 17 00:00:00 2001 From: Drew Boyuka Date: Fri, 21 Apr 2023 23:23:48 -0400 Subject: [PATCH] Fix two bugs with unsafe memory access (#253) * 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. --- codec_record.go | 6 ++--- decoder_record_test.go | 61 +++++++++++++++++++++++++++++++++++++----- encoder_record_test.go | 19 +++++++------ types_test.go | 15 ++++++----- 4 files changed, 77 insertions(+), 24 deletions(-) diff --git a/codec_record.go b/codec_record.go index 5650dfce..9638a709 100644 --- a/codec_record.go +++ b/codec_record.go @@ -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 } @@ -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 } diff --git a/decoder_record_test.go b/decoder_record_test.go index 9ed1cca0..0d1d23f4 100644 --- a/decoder_record_test.go +++ b/decoder_record_test.go @@ -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)) @@ -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)) @@ -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) { diff --git a/encoder_record_test.go b/encoder_record_test.go index 149d92f3..73c28e4b 100644 --- a/encoder_record_test.go +++ b/encoder_record_test.go @@ -190,10 +190,11 @@ 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) @@ -201,7 +202,7 @@ func TestEncoder_RecordEmbeddedStruct(t *testing.T) { 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) { @@ -212,10 +213,11 @@ 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) @@ -223,7 +225,7 @@ func TestEncoder_RecordEmbeddedPtrStruct(t *testing.T) { 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) { @@ -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) diff --git a/types_test.go b/types_test.go index eb864bd7..020369eb 100644 --- a/types_test.go +++ b/types_test.go @@ -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 {