Repository: hamba/avro
Pull request #253 (2023-04-21 23:54:50)

Fix two bugs with unsafe memory access
See first commit for unit tests demonstrating the issues. Run the tests with race detector on (`go test -race`).

Bug 1: the struct encoder and decoder both do nil-check on `ptr` (pointer to top-level struct) rather than `fieldPtr` (pointer to current field) when looking at an struct-embedded pointer.

This was not caught by the test suite because the embedded pointer struct tested is at the top of the struct, so these two variables happen to be the same.

Bug 2: given these types:
```
type Outer struct { *Inner }
type Inner struct { Data [256]byte }
```
if the decoder is decoding an `Outer` and sees a `nil` pointer for `*Inner`, when it tries to allocate a block of memory sized for `Inner` (size 256 bytes), it mistakenly allocates a block sized for `*Inner` instead (size 8), leading to to buffer overruns on later access.

This was not caught by the test suite because the embedded struct tested has the same size as a pointer. Even with a larger struct (provided in this PR), it only fails sporadically, requiring the race detector or a high test run count to trigger reliably.

daboyuka: CC @mibanescu 

Patch:
diff --git a/codec_record.go b/codec_record.go
index 5650dfc..9638a70 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
 				}


Test patch:
diff --git a/decoder_record_test.go b/decoder_record_test.go
index 9ed1cca..0d1d23f 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 149d92f..73c28e4 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 eb864bd..020369e 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 {