[mem store] Return clones of spans to prevent in-place changes by adj…

…usters (#2720) * Copy spans from memory store, fixes #2719 Copying allows spans to be freely modified by adjusters and any other code without accidentally altering what is stored in the in-memory store itself. Signed-off-by: Ivan Babrou <[email protected]> * Add tests to exercise the broken serialization path Signed-off-by: Ivan Babrou <[email protected]>
jaegertracing · Jan 9, 2021 · 2f8ffa9 · 2f8ffa9
1 parent e788e55
commit 2f8ffa9
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 9 deletions.
diff --git a/plugin/storage/memory/memory.go b/plugin/storage/memory/memory.go
@@ -22,6 +22,8 @@ import (
 	"sync"
 	"time"
 
+	"github.com/golang/protobuf/proto"
+
 	"github.com/jaegertracing/jaeger/model"
 	"github.com/jaegertracing/jaeger/model/adjuster"
 	"github.com/jaegertracing/jaeger/pkg/memory/config"
@@ -164,15 +166,19 @@ func (m *Store) GetTrace(ctx context.Context, traceID model.TraceID) (*model.Tra
 	if !ok {
 		return nil, spanstore.ErrTraceNotFound
 	}
-	return m.copyTrace(trace), nil
+	return m.copyTrace(trace)
 }
 
 // Spans may still be added to traces after they are returned to user code, so make copies.
-func (m *Store) copyTrace(trace *model.Trace) *model.Trace {
-	return &model.Trace{
-		Spans:    append([]*model.Span(nil), trace.Spans...),
-		Warnings: append([]string(nil), trace.Warnings...),
+func (m *Store) copyTrace(trace *model.Trace) (*model.Trace, error) {
+	bytes, err := proto.Marshal(trace)
+	if err != nil {
+		return nil, err
 	}
+
+	copied := &model.Trace{}
+	err = proto.Unmarshal(bytes, copied)
+	return copied, err
 }
 
 // GetServices returns a list of all known services
@@ -211,7 +217,12 @@ func (m *Store) FindTraces(ctx context.Context, query *spanstore.TraceQueryParam
 	var retMe []*model.Trace
 	for _, trace := range m.traces {
 		if m.validTrace(trace, query) {
-			retMe = append(retMe, m.copyTrace(trace))
+			copied, err := m.copyTrace(trace)
+			if err != nil {
+				return nil, err
+			}
+
+			retMe = append(retMe, copied)
 		}
 	}
 

diff --git a/plugin/storage/memory/memory_test.go b/plugin/storage/memory/memory_test.go
@@ -34,7 +34,7 @@ var testingSpan = &model.Span{
 	SpanID:  model.NewSpanID(1),
 	Process: &model.Process{
 		ServiceName: "serviceName",
-		Tags:        model.KeyValues{},
+		Tags:        []model.KeyValue(nil),
 	},
 	OperationName: "operationName",
 	Tags: model.KeyValues{
@@ -43,14 +43,14 @@ var testingSpan = &model.Span{
 	},
 	Logs: []model.Log{
 		{
-			Timestamp: time.Now(),
+			Timestamp: time.Now().UTC(),
 			Fields: []model.KeyValue{
 				model.String("logKey", "logValue"),
 			},
 		},
 	},
 	Duration:  time.Second * 5,
-	StartTime: time.Unix(300, 0),
+	StartTime: time.Unix(300, 0).UTC(),
 }
 
 var childSpan1 = &model.Span{
@@ -128,6 +128,14 @@ var childSpan2_1 = &model.Span{
 	StartTime: time.Unix(300, 0),
 }
 
+// This kind of trace cannot be serialized
+var nonSerializableSpan = &model.Span{
+	Process: &model.Process{
+		ServiceName: "naughtyService",
+	},
+	StartTime: time.Date(0, 0, 0, 0, 0, 0, 0, time.UTC),
+}
+
 func withPopulatedMemoryStore(f func(store *Store)) {
 	memStore := NewStore()
 	memStore.WriteSpan(context.Background(), testingSpan)
@@ -210,6 +218,34 @@ func TestStoreGetTraceSuccess(t *testing.T) {
 	})
 }
 
+func TestStoreGetAndMutateTrace(t *testing.T) {
+	withPopulatedMemoryStore(func(store *Store) {
+		trace, err := store.GetTrace(context.Background(), testingSpan.TraceID)
+		assert.NoError(t, err)
+		assert.Len(t, trace.Spans, 1)
+		assert.Equal(t, testingSpan, trace.Spans[0])
+		assert.Len(t, trace.Spans[0].Warnings, 0)
+
+		trace.Spans[0].Warnings = append(trace.Spans[0].Warnings, "the end is near")
+
+		trace, err = store.GetTrace(context.Background(), testingSpan.TraceID)
+		assert.NoError(t, err)
+		assert.Len(t, trace.Spans, 1)
+		assert.Equal(t, testingSpan, trace.Spans[0])
+		assert.Len(t, trace.Spans[0].Warnings, 0)
+	})
+}
+
+func TestStoreGetTraceError(t *testing.T) {
+	withPopulatedMemoryStore(func(store *Store) {
+		store.traces[testingSpan.TraceID] = &model.Trace{
+			Spans: []*model.Span{nonSerializableSpan},
+		}
+		_, err := store.GetTrace(context.Background(), testingSpan.TraceID)
+		assert.Error(t, err)
+	})
+}
+
 func TestStoreGetTraceFailure(t *testing.T) {
 	withPopulatedMemoryStore(func(store *Store) {
 		trace, err := store.GetTrace(context.Background(), model.TraceID{})
@@ -282,6 +318,15 @@ func TestStoreGetEmptyTraceSet(t *testing.T) {
 	})
 }
 
+func TestStoreFindTracesError(t *testing.T) {
+	withPopulatedMemoryStore(func(store *Store) {
+		err := store.WriteSpan(context.Background(), nonSerializableSpan)
+		assert.NoError(t, err)
+		_, err = store.FindTraces(context.Background(), &spanstore.TraceQueryParameters{ServiceName: "naughtyService"})
+		assert.Error(t, err)
+	})
+}
+
 func TestStoreFindTracesLimitGetsMostRecent(t *testing.T) {
 	storeSize, querySize := 100, 10