From 5b777f8a8cac32acef0b7d1274809383361c7fa2 Mon Sep 17 00:00:00 2001 From: Nick Van Wiggeren Date: Tue, 6 Jan 2026 21:29:03 +0000 Subject: [PATCH 1/2] binlog_json: fix opaque value parsing to read variable-length size instead of assuming 8 bytes Signed-off-by: Nick Van Wiggeren --- go/mysql/binlog/binlog_json.go | 31 +++++++++++++--- go/mysql/binlog/binlog_json_test.go | 56 ++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 17 deletions(-) diff --git a/go/mysql/binlog/binlog_json.go b/go/mysql/binlog/binlog_json.go index 51b4fef0ef8..e012672b8ab 100644 --- a/go/mysql/binlog/binlog_json.go +++ b/go/mysql/binlog/binlog_json.go @@ -449,11 +449,23 @@ func binparserLiteral(_ jsonDataType, data []byte, pos int) (node *json.Value, e // other types are stored as catch-all opaque types: documentation on these is scarce. // we currently know about (and support) date/time/datetime/decimal. func binparserOpaque(_ jsonDataType, data []byte, pos int) (node *json.Value, err error) { + if pos >= len(data) { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque value missing type at position %d", pos) + } dataType := data[pos] - start := 3 // account for length of stored value - end := start + 8 // all currently supported opaque data types are 8 bytes in size + if pos+1 >= len(data) { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque value missing length at position %d", pos) + } + length, start := readVariableLength(data, pos+1) + end := start + length + if start > len(data) || end > len(data) { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque value length %d exceeds available bytes", length) + } switch dataType { case TypeDate: + if length < 8 { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque date length %d is too short", length) + } raw := binary.LittleEndian.Uint64(data[start:end]) value := raw >> 24 yearMonth := (value >> 22) & 0x01ffff // 17 bits starting at 22nd @@ -463,6 +475,9 @@ func binparserOpaque(_ jsonDataType, data []byte, pos int) (node *json.Value, er dateString := fmt.Sprintf("%04d-%02d-%02d", year, month, day) node = json.NewDate(dateString) case TypeTime2, TypeTime: + if length < 8 { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque time length %d is too short", length) + } raw := binary.LittleEndian.Uint64(data[start:end]) value := raw >> 24 hour := (value >> 12) & 0x03ff // 10 bits starting at 12th @@ -472,6 +487,9 @@ func binparserOpaque(_ jsonDataType, data []byte, pos int) (node *json.Value, er timeString := fmt.Sprintf("%02d:%02d:%02d.%06d", hour, minute, second, microSeconds) node = json.NewTime(timeString) case TypeDateTime2, TypeDateTime, TypeTimestamp2, TypeTimestamp: + if length < 8 { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque datetime length %d is too short", length) + } raw := binary.LittleEndian.Uint64(data[start:end]) value := raw >> 24 yearMonth := (value >> 22) & 0x01ffff // 17 bits starting at 22nd @@ -485,6 +503,9 @@ func binparserOpaque(_ jsonDataType, data []byte, pos int) (node *json.Value, er timeString := fmt.Sprintf("%04d-%02d-%02d %02d:%02d:%02d.%06d", year, month, day, hour, minute, second, microSeconds) node = json.NewDateTime(timeString) case TypeDecimal, TypeNewDecimal: + if length < 2 { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque decimal length %d is too short", length) + } decimalData := data[start:end] precision := decimalData[0] scale := decimalData[1] @@ -495,11 +516,11 @@ func binparserOpaque(_ jsonDataType, data []byte, pos int) (node *json.Value, er } node = json.NewNumber(val.ToString(), json.NumberTypeDecimal) case TypeVarchar, TypeVarString, TypeString, TypeBlob, TypeTinyBlob, TypeMediumBlob, TypeLongBlob: - node = json.NewBlob(string(data[pos+1:])) + node = json.NewBlob(string(data[start:end])) case TypeBit: - node = json.NewBit(string(data[pos+1:])) + node = json.NewBit(string(data[start:end])) default: - node = json.NewOpaqueValue(string(data[pos+1:])) + node = json.NewOpaqueValue(string(data[start:end])) } return node, nil } diff --git a/go/mysql/binlog/binlog_json_test.go b/go/mysql/binlog/binlog_json_test.go index 5652b58567e..e3a9f66c00c 100644 --- a/go/mysql/binlog/binlog_json_test.go +++ b/go/mysql/binlog/binlog_json_test.go @@ -232,19 +232,24 @@ func TestBinaryJSON(t *testing.T) { expected: json.NewNumber("123456789.1234", json.NumberTypeDecimal), }, { - name: `bit literal [2 202 254]`, + name: `small decimal "1.99"`, + data: []byte{15, 246, 4, 3, 2, 0x81, 0x63}, + expected: json.NewNumber("1.99", json.NumberTypeDecimal), + }, + { + name: `bit literal 0xCAFE`, data: []byte{15, 16, 2, 202, 254}, - expected: json.NewBit(string([]byte{2, 202, 254})), + expected: json.NewBit(string([]byte{202, 254})), }, { - name: `opaque string [2 202 254]`, + name: `opaque string 0xCAFE`, data: []byte{15, 15, 2, 202, 254}, - expected: json.NewBlob(string([]byte{2, 202, 254})), + expected: json.NewBlob(string([]byte{202, 254})), }, { - name: `opaque blob [2 202 254]`, + name: `opaque blob 0xCAFE`, data: []byte{15, 252, 2, 202, 254}, - expected: json.NewBlob(string([]byte{2, 202, 254})), + expected: json.NewBlob(string([]byte{202, 254})), }, } for _, tc := range testcases { @@ -256,6 +261,32 @@ func TestBinaryJSON(t *testing.T) { } } +func TestBinaryJSONOpaqueErrors(t *testing.T) { + testcases := []struct { + name string + data []byte + }{ + { + name: "opaque length exceeds payload", + data: []byte{15, 252, 2, 202}, + }, + { + name: "opaque date too short", + data: []byte{15, 10, 4, 0, 0, 0, 0}, + }, + { + name: "opaque decimal too short", + data: []byte{15, 246, 1, 0x01}, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + _, err := ParseBinaryJSON(tc.data) + require.Error(t, err) + }) + } +} + func TestMarshalJSONToSQL(t *testing.T) { testcases := []struct { name string @@ -443,19 +474,20 @@ func TestMarshalJSONToSQL(t *testing.T) { expected: `CAST(123456789.1234 as JSON)`, }, { - name: `bit literal [2 202 254]`, + // 0xCAFE = 51966 = binary 1100101011111110 (16 bits) + name: `bit literal 0xCAFE`, data: []byte{15, 16, 2, 202, 254}, - expected: `CAST(b'101100101011111110' as JSON)`, + expected: `CAST(b'1100101011111110' as JSON)`, }, { - name: `opaque string [2 202 254]`, + name: `opaque string 0xCAFE`, data: []byte{15, 15, 2, 202, 254}, - expected: `CAST(x'02CAFE' as JSON)`, + expected: `CAST(x'CAFE' as JSON)`, }, { - name: `opaque blob [2 202 254]`, + name: `opaque blob 0xCAFE`, data: []byte{15, 252, 2, 202, 254}, - expected: `CAST(x'02CAFE' as JSON)`, + expected: `CAST(x'CAFE' as JSON)`, }, } for _, tc := range testcases { From 68383cde2bedcb31822e3c3dc5e3a54a536a0220 Mon Sep 17 00:00:00 2001 From: Nick Van Wiggeren Date: Wed, 7 Jan 2026 10:09:09 -0800 Subject: [PATCH 2/2] code review feedback Signed-off-by: Nick Van Wiggeren --- go/mysql/binlog/binlog_json.go | 14 ++++++++------ go/mysql/binlog/binlog_json_test.go | 22 +++++++++++++--------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/go/mysql/binlog/binlog_json.go b/go/mysql/binlog/binlog_json.go index e012672b8ab..f647a4f130e 100644 --- a/go/mysql/binlog/binlog_json.go +++ b/go/mysql/binlog/binlog_json.go @@ -450,16 +450,18 @@ func binparserLiteral(_ jsonDataType, data []byte, pos int) (node *json.Value, e // we currently know about (and support) date/time/datetime/decimal. func binparserOpaque(_ jsonDataType, data []byte, pos int) (node *json.Value, err error) { if pos >= len(data) { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque value missing type at position %d", pos) + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque JSON field value missing type at position %d", pos) } - dataType := data[pos] - if pos+1 >= len(data) { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque value missing length at position %d", pos) + typePos := pos + dataType := data[typePos] + pos = typePos + 1 + if pos >= len(data) { + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque JSON field value missing length at position %d", typePos) } - length, start := readVariableLength(data, pos+1) + length, start := readVariableLength(data, pos) end := start + length if start > len(data) || end > len(data) { - return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque value length %d exceeds available bytes", length) + return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "opaque JSON field value length %d exceeds available bytes", length) } switch dataType { case TypeDate: diff --git a/go/mysql/binlog/binlog_json_test.go b/go/mysql/binlog/binlog_json_test.go index e3a9f66c00c..59cfffcbab9 100644 --- a/go/mysql/binlog/binlog_json_test.go +++ b/go/mysql/binlog/binlog_json_test.go @@ -263,26 +263,30 @@ func TestBinaryJSON(t *testing.T) { func TestBinaryJSONOpaqueErrors(t *testing.T) { testcases := []struct { - name string - data []byte + name string + data []byte + expectedErr string }{ { - name: "opaque length exceeds payload", - data: []byte{15, 252, 2, 202}, + name: "opaque length exceeds payload", + data: []byte{15, 252, 2, 202}, + expectedErr: "opaque JSON field value length 2 exceeds available bytes", }, { - name: "opaque date too short", - data: []byte{15, 10, 4, 0, 0, 0, 0}, + name: "opaque date too short", + data: []byte{15, 10, 4, 0, 0, 0, 0}, + expectedErr: "opaque date length 4 is too short", }, { - name: "opaque decimal too short", - data: []byte{15, 246, 1, 0x01}, + name: "opaque decimal too short", + data: []byte{15, 246, 1, 0x01}, + expectedErr: "opaque decimal length 1 is too short", }, } for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { _, err := ParseBinaryJSON(tc.data) - require.Error(t, err) + require.ErrorContains(t, err, tc.expectedErr) }) } }