Merge pull request #74 from paullegranddc/paullgdc/fix_from_proto_nil…

…_deref Fix nullable pointer dereference in FromProto
DataDog · Oct 10, 2023 · 83610c4 · 83610c4
2 parents 3b624d1 + 96d776b
commit 83610c4
Show file tree

Hide file tree

Showing 4 changed files with 102 additions and 22 deletions.
diff --git a/ddsketch/ddsketch.go b/ddsketch/ddsketch.go
@@ -262,13 +262,13 @@ func (s *DDSketch) GetSum() (sum float64) {
 
 // GetPositiveValueStore returns the store.Store object that contains the positive
 // values of the sketch.
-func (s *DDSketch) GetPositiveValueStore() (store.Store) {
+func (s *DDSketch) GetPositiveValueStore() store.Store {
 	return s.positiveValueStore
 }
 
 // GetNegativeValueStore returns the store.Store object that contains the negative
 // values of the sketch.
-func (s *DDSketch) GetNegativeValueStore() (store.Store) {
+func (s *DDSketch) GetNegativeValueStore() store.Store {
 	return s.negativeValueStore
 }
 
@@ -320,9 +320,13 @@ func FromProto(pb *sketchpb.DDSketch) (*DDSketch, error) {
 
 func FromProtoWithStoreProvider(pb *sketchpb.DDSketch, storeProvider store.Provider) (*DDSketch, error) {
 	positiveValueStore := storeProvider()
-	store.MergeWithProto(positiveValueStore, pb.PositiveValues)
+	if pb.PositiveValues != nil {
+		store.MergeWithProto(positiveValueStore, pb.PositiveValues)
+	}
 	negativeValueStore := storeProvider()
-	store.MergeWithProto(negativeValueStore, pb.NegativeValues)
+	if pb.NegativeValues != nil {
+		store.MergeWithProto(negativeValueStore, pb.NegativeValues)
+	}
 	m, err := mapping.FromProto(pb.Mapping)
 	if err != nil {
 		return nil, err
@@ -559,13 +563,13 @@ func (s *DDSketchWithExactSummaryStatistics) GetSum() float64 {
 
 // GetPositiveValueStore returns the store.Store object that contains the positive
 // values of the sketch.
-func (s *DDSketchWithExactSummaryStatistics) GetPositiveValueStore() (store.Store) {
+func (s *DDSketchWithExactSummaryStatistics) GetPositiveValueStore() store.Store {
 	return s.DDSketch.positiveValueStore
 }
 
 // GetNegativeValueStore returns the store.Store object that contains the negative
 // values of the sketch.
-func (s *DDSketchWithExactSummaryStatistics) GetNegativeValueStore() (store.Store) {
+func (s *DDSketchWithExactSummaryStatistics) GetNegativeValueStore() store.Store {
 	return s.DDSketch.negativeValueStore
 }
 

diff --git a/ddsketch/ddsketch_test.go b/ddsketch/ddsketch_test.go
@@ -6,11 +6,12 @@
 package ddsketch
 
 import (
-	"github.com/DataDog/sketches-go/ddsketch/stat"
 	"math"
 	"math/rand"
 	"testing"
 
+	"github.com/DataDog/sketches-go/ddsketch/stat"
+
 	"github.com/DataDog/sketches-go/dataset"
 	"github.com/DataDog/sketches-go/ddsketch/mapping"
 	"github.com/DataDog/sketches-go/ddsketch/pb/sketchpb"
@@ -726,9 +727,10 @@ func TestBenchmarkEncodedSize(t *testing.T) {
 }
 
 type serTestCase struct {
-	name  string
-	ser   func(s *DDSketch, b *[]byte)
-	deser func(b []byte, s *DDSketch, p store.Provider)
+	name        string
+	ser         func(s *DDSketch, b *[]byte)
+	deser       func(b []byte, s *DDSketch, p store.Provider) error
+	expectedErr string
 }
 
 var serTestCases []serTestCase = []serTestCase{
@@ -738,23 +740,82 @@ var serTestCases []serTestCase = []serTestCase{
 			serialized, _ := proto.Marshal(s.ToProto())
 			*b = serialized
 		},
-		deser: func(b []byte, s *DDSketch, p store.Provider) {
+		deser: func(b []byte, s *DDSketch, p store.Provider) error {
+			var sketchPb sketchpb.DDSketch
+			proto.Unmarshal(b, &sketchPb)
+
+			serialized, err := FromProtoWithStoreProvider(&sketchPb, p)
+			*s = *serialized
+			return err
+		},
+	},
+	{
+		name: "proto_nil_positive_values",
+		ser: func(s *DDSketch, b *[]byte) {
+			s.positiveValueStore.Clear()
+			pb := s.ToProto()
+			pb.PositiveValues = nil
+			serialized, _ := proto.Marshal(pb)
+			*b = serialized
+		},
+		deser: func(b []byte, s *DDSketch, p store.Provider) error {
 			var sketchPb sketchpb.DDSketch
 			proto.Unmarshal(b, &sketchPb)
 
-			serialized, _ := FromProtoWithStoreProvider(&sketchPb, p)
+			serialized, err := FromProtoWithStoreProvider(&sketchPb, p)
 			*s = *serialized
+			return err
 		},
 	},
+	{
+		name: "proto_nil_negative_values",
+		ser: func(s *DDSketch, b *[]byte) {
+			s.negativeValueStore.Clear()
+			pb := s.ToProto()
+			pb.NegativeValues = nil
+			serialized, _ := proto.Marshal(pb)
+			*b = serialized
+		},
+		deser: func(b []byte, s *DDSketch, p store.Provider) error {
+			var sketchPb sketchpb.DDSketch
+			proto.Unmarshal(b, &sketchPb)
+
+			serialized, err := FromProtoWithStoreProvider(&sketchPb, p)
+			*s = *serialized
+			return err
+		},
+	},
+	{
+		name: "proto_nil_mapping",
+		ser: func(s *DDSketch, b *[]byte) {
+			pb := s.ToProto()
+			pb.Mapping = nil
+			serialized, _ := proto.Marshal(pb)
+			*b = serialized
+		},
+		deser: func(b []byte, s *DDSketch, p store.Provider) error {
+			var sketchPb sketchpb.DDSketch
+			proto.Unmarshal(b, &sketchPb)
+
+			serialized, err := FromProtoWithStoreProvider(&sketchPb, p)
+			if err != nil {
+				return err
+			}
+			*s = *serialized
+			return nil
+		},
+		expectedErr: "cannot create IndexMapping from nil protobuf index mapping",
+	},
 	{
 		name: "custom",
 		ser: func(s *DDSketch, b *[]byte) {
 			*b = []byte{}
 			s.Encode(b, false)
 		},
-		deser: func(b []byte, s *DDSketch, p store.Provider) {
-			sketch, _ := DecodeDDSketch(b, p, nil)
+		deser: func(b []byte, s *DDSketch, p store.Provider) error {
+			sketch, err := DecodeDDSketch(b, p, nil)
 			*s = *sketch
+			return err
 		},
 	},
 	{
@@ -763,9 +824,9 @@ var serTestCases []serTestCase = []serTestCase{
 			*b = (*b)[:0]
 			s.Encode(b, false)
 		},
-		deser: func(b []byte, s *DDSketch, p store.Provider) {
+		deser: func(b []byte, s *DDSketch, p store.Provider) error {
 			s.Clear()
-			s.DecodeAndMergeWith(b)
+			return s.DecodeAndMergeWith(b)
 		},
 	},
 }
@@ -777,14 +838,20 @@ func TestSerDeser(t *testing.T) {
 		store.SparseStoreConstructor,
 	}
 	for _, testCase := range dataTestCases {
-		sketch := NewDDSketchFromStoreProvider(testCase.indexMapping, testCase.storeProvider)
-		testCase.fillSketch(*sketch)
 		for _, serTestCase := range serTestCases {
-			var serialized []byte
-			serTestCase.ser(sketch, &serialized)
 			for _, storeProvider := range storeProviders {
+				sketch := NewDDSketchFromStoreProvider(testCase.indexMapping, testCase.storeProvider)
+				testCase.fillSketch(*sketch)
+
+				var serialized []byte
+				serTestCase.ser(sketch, &serialized)
+
 				deserialized := NewDDSketchFromStoreProvider(sketch.IndexMapping, storeProvider)
-				serTestCase.deser(serialized, deserialized, storeProvider)
+				err := serTestCase.deser(serialized, deserialized, storeProvider)
+				if serTestCase.expectedErr != "" {
+					assert.EqualError(t, err, serTestCase.expectedErr)
+					continue
+				}
 				assertSketchesEquivalent(t, sketch, deserialized)
 			}
 		}

diff --git a/ddsketch/mapping/index_mapping.go b/ddsketch/mapping/index_mapping.go
@@ -39,6 +39,9 @@ func NewDefaultMapping(relativeAccuracy float64) (IndexMapping, error) {
 
 // FromProto returns an Index mapping from the protobuf definition of it
 func FromProto(m *sketchpb.IndexMapping) (IndexMapping, error) {
+	if m == nil {
+		return nil, errors.New("cannot create IndexMapping from nil protobuf index mapping")
+	}
 	switch m.Interpolation {
 	case sketchpb.IndexMapping_NONE:
 		return NewLogarithmicMappingWithGamma(m.Gamma, m.IndexOffset)

diff --git a/ddsketch/mapping/index_mapping_test.go b/ddsketch/mapping/index_mapping_test.go
@@ -6,10 +6,11 @@
 package mapping
 
 import (
-	"github.com/DataDog/sketches-go/ddsketch/encoding"
 	"math"
 	"testing"
 
+	"github.com/DataDog/sketches-go/ddsketch/encoding"
+
 	"github.com/stretchr/testify/assert"
 )
 
@@ -109,6 +110,11 @@ func TestSerialization(t *testing.T) {
 	assert.True(t, m.Equals(deserializedMapping))
 }
 
+func TestDeserializationNil(t *testing.T) {
+	_, err := FromProto(nil)
+	assert.EqualError(t, err, "cannot create IndexMapping from nil protobuf index mapping")
+}
+
 func TestEncodeDecodeEquality(t *testing.T) {
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {