Fix gosec overflow alerts (#5799)

To allow the golangci-lint upgrade in #5796. --------- Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
open-telemetry · Sep 13, 2024 · a3c512a · a3c512a
1 parent 5e3434c
commit a3c512a
Show file tree

Hide file tree

Showing 26 changed files with 184 additions and 50 deletions.
diff --git a/attribute/value_test.go b/attribute/value_test.go
@@ -38,6 +38,12 @@ func TestValue(t *testing.T) {
 			wantType:  attribute.INT64,
 			wantValue: int64(42),
 		},
+		{
+			name:      "Key.Int64() correctly returns negative keys's internal int64 value",
+			value:     k.Int64(-42).Value,
+			wantType:  attribute.INT64,
+			wantValue: int64(-42),
+		},
 		{
 			name:      "Key.Int64Slice() correctly returns keys's internal []int64 value",
 			value:     k.Int64Slice([]int64{42, -3, 12}).Value,

diff --git a/bridge/opencensus/internal/ocmetric/metric.go b/bridge/opencensus/internal/ocmetric/metric.go
@@ -144,7 +144,7 @@ func convertHistogram(labelKeys []ocmetricdata.LabelKey, ts []*ocmetricdata.Time
 				Attributes:   attrs,
 				StartTime:    t.StartTime,
 				Time:         p.Time,
-				Count:        uint64(dist.Count),
+				Count:        uint64(max(0, dist.Count)), // nolint:gosec // A count should never be negative.
 				Sum:          dist.Sum,
 				Bounds:       dist.BucketOptions.Bounds,
 				BucketCounts: bucketCounts,
@@ -166,7 +166,7 @@ func convertBuckets(buckets []ocmetricdata.Bucket) ([]uint64, []metricdata.Exemp
 			err = errors.Join(err, fmt.Errorf("%w: %q", errNegativeBucketCount, bucket.Count))
 			continue
 		}
-		bucketCounts[i] = uint64(bucket.Count)
+		bucketCounts[i] = uint64(max(0, bucket.Count)) // nolint:gosec // A count should never be negative.
 
 		if bucket.Exemplar != nil {
 			exemplar, exemplarErr := convertExemplar(bucket.Exemplar)
@@ -357,7 +357,7 @@ func convertSummary(labelKeys []ocmetricdata.LabelKey, ts []*ocmetricdata.TimeSe
 				Attributes:     attrs,
 				StartTime:      t.StartTime,
 				Time:           p.Time,
-				Count:          uint64(summary.Count),
+				Count:          uint64(max(0, summary.Count)), // nolint:gosec // A count should never be negative.
 				QuantileValues: convertQuantiles(summary.Snapshot),
 				Sum:            summary.Sum,
 			}

diff --git a/bridge/opencensus/internal/span.go b/bridge/opencensus/internal/span.go
@@ -61,7 +61,7 @@ func (s *Span) SetName(name string) {
 
 // SetStatus sets the status of this span, if it is recording events.
 func (s *Span) SetStatus(status octrace.Status) {
-	s.otelSpan.SetStatus(codes.Code(status.Code), status.Message)
+	s.otelSpan.SetStatus(codes.Code(max(0, status.Code)), status.Message) // nolint:gosec // Overflow checked.
 }
 
 // AddAttributes sets attributes in this span.

diff --git a/bridge/opencensus/internal/span_test.go b/bridge/opencensus/internal/span_test.go
@@ -96,15 +96,40 @@ func TestSpanSetStatus(t *testing.T) {
 	s := &span{recording: true}
 	ocS := internal.NewSpan(s)
 
-	c, d := codes.Error, "error"
-	status := octrace.Status{Code: int32(c), Message: d}
-	ocS.SetStatus(status)
+	for _, tt := range []struct {
+		name string
 
-	if s.sCode != c {
-		t.Error("span.SetStatus failed to set OpenTelemetry status code")
-	}
-	if s.sMsg != d {
-		t.Error("span.SetStatus failed to set OpenTelemetry status description")
+		code    int32
+		message string
+
+		wantCode codes.Code
+	}{
+		{
+			name:    "with an error code",
+			code:    int32(codes.Error),
+			message: "error",
+
+			wantCode: codes.Error,
+		},
+		{
+			name:    "with a negative/invalid code",
+			code:    -42,
+			message: "error",
+
+			wantCode: codes.Unset,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			status := octrace.Status{Code: tt.code, Message: tt.message}
+			ocS.SetStatus(status)
+
+			if s.sCode != tt.wantCode {
+				t.Errorf("span.SetStatus failed to set OpenTelemetry status code. Expected %d, got %d", tt.wantCode, s.sCode)
+			}
+			if s.sMsg != tt.message {
+				t.Errorf("span.SetStatus failed to set OpenTelemetry status description. Expected %s, got %s", tt.message, s.sMsg)
+			}
+		})
 	}
 }
 

diff --git a/exporters/otlp/otlplog/otlploggrpc/internal/transform/log.go b/exporters/otlp/otlplog/otlploggrpc/internal/transform/log.go
@@ -139,10 +139,11 @@ func LogRecord(record log.Record) *lpb.LogRecord {
 // year 1678 or after 2262). timeUnixNano on the zero Time returns 0. The
 // result does not depend on the location associated with t.
 func timeUnixNano(t time.Time) uint64 {
-	if t.IsZero() {
+	nano := t.UnixNano()
+	if nano < 0 {
 		return 0
 	}
-	return uint64(t.UnixNano())
+	return uint64(nano) // nolint:gosec // Overflow checked.
 }
 
 // AttrIter transforms an [attribute.Iterator] into OTLP key-values.

diff --git a/exporters/otlp/otlplog/otlploggrpc/internal/transform/log_test.go b/exporters/otlp/otlplog/otlploggrpc/internal/transform/log_test.go
@@ -30,6 +30,9 @@ var (
 	ts  = time.Date(2000, time.January, 0o1, 0, 0, 0, 0, time.FixedZone("GMT", 0))
 	obs = ts.Add(30 * time.Second)
 
+	// A time before unix 0.
+	negativeTs = time.Date(1969, 7, 20, 20, 17, 0, 0, time.UTC)
+
 	alice = api.String("user", "alice")
 	bob   = api.String("user", "bob")
 
@@ -158,6 +161,20 @@ var (
 			Resource:             res,
 		}.NewRecord())
 
+		out = append(out, logtest.RecordFactory{
+			Timestamp:            negativeTs,
+			ObservedTimestamp:    obs,
+			Severity:             sevB,
+			SeverityText:         "B",
+			Body:                 bodyB,
+			Attributes:           []api.KeyValue{bob},
+			TraceID:              trace.TraceID(traceIDB),
+			SpanID:               trace.SpanID(spanIDB),
+			TraceFlags:           trace.TraceFlags(flagsB),
+			InstrumentationScope: &scope,
+			Resource:             res,
+		}.NewRecord())
+
 		return out
 	}()
 
@@ -206,6 +223,17 @@ var (
 			TraceId:              traceIDB,
 			SpanId:               spanIDB,
 		},
+		{
+			TimeUnixNano:         0,
+			ObservedTimeUnixNano: uint64(obs.UnixNano()),
+			SeverityNumber:       pbSevB,
+			SeverityText:         "B",
+			Body:                 pbBodyB,
+			Attributes:           []*cpb.KeyValue{pbBob},
+			Flags:                uint32(flagsB),
+			TraceId:              traceIDB,
+			SpanId:               spanIDB,
+		},
 	}
 
 	pbScopeLogs = &lpb.ScopeLogs{

diff --git a/exporters/otlp/otlplog/otlploghttp/internal/transform/log.go b/exporters/otlp/otlplog/otlploghttp/internal/transform/log.go
@@ -139,10 +139,11 @@ func LogRecord(record log.Record) *lpb.LogRecord {
 // year 1678 or after 2262). timeUnixNano on the zero Time returns 0. The
 // result does not depend on the location associated with t.
 func timeUnixNano(t time.Time) uint64 {
-	if t.IsZero() {
+	nano := t.UnixNano()
+	if nano < 0 {
 		return 0
 	}
-	return uint64(t.UnixNano())
+	return uint64(nano) // nolint:gosec // Overflow checked.
 }
 
 // AttrIter transforms an [attribute.Iterator] into OTLP key-values.

diff --git a/exporters/otlp/otlplog/otlploghttp/internal/transform/log_test.go b/exporters/otlp/otlplog/otlploghttp/internal/transform/log_test.go
@@ -30,6 +30,9 @@ var (
 	ts  = time.Date(2000, time.January, 0o1, 0, 0, 0, 0, time.FixedZone("GMT", 0))
 	obs = ts.Add(30 * time.Second)
 
+	// A time before unix 0.
+	negativeTs = time.Date(1969, 7, 20, 20, 17, 0, 0, time.UTC)
+
 	alice = api.String("user", "alice")
 	bob   = api.String("user", "bob")
 
@@ -158,6 +161,20 @@ var (
 			Resource:             res,
 		}.NewRecord())
 
+		out = append(out, logtest.RecordFactory{
+			Timestamp:            negativeTs,
+			ObservedTimestamp:    obs,
+			Severity:             sevB,
+			SeverityText:         "B",
+			Body:                 bodyB,
+			Attributes:           []api.KeyValue{bob},
+			TraceID:              trace.TraceID(traceIDB),
+			SpanID:               trace.SpanID(spanIDB),
+			TraceFlags:           trace.TraceFlags(flagsB),
+			InstrumentationScope: &scope,
+			Resource:             res,
+		}.NewRecord())
+
 		return out
 	}()
 
@@ -206,6 +223,17 @@ var (
 			TraceId:              traceIDB,
 			SpanId:               spanIDB,
 		},
+		{
+			TimeUnixNano:         0,
+			ObservedTimeUnixNano: uint64(obs.UnixNano()),
+			SeverityNumber:       pbSevB,
+			SeverityText:         "B",
+			Body:                 pbBodyB,
+			Attributes:           []*cpb.KeyValue{pbBob},
+			Flags:                uint32(flagsB),
+			TraceId:              traceIDB,
+			SpanId:               spanIDB,
+		},
 	}
 
 	pbScopeLogs = &lpb.ScopeLogs{

diff --git a/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/transform/metricdata.go b/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/transform/metricdata.go
@@ -279,10 +279,7 @@ func Temporality(t metricdata.Temporality) (mpb.AggregationTemporality, error) {
 // timeUnixNano on the zero Time returns 0.
 // The result does not depend on the location associated with t.
 func timeUnixNano(t time.Time) uint64 {
-	if t.IsZero() {
-		return 0
-	}
-	return uint64(t.UnixNano())
+	return uint64(max(0, t.UnixNano())) // nolint:gosec // Overflow checked.
 }
 
 // Exemplars returns a slice of OTLP Exemplars generated from exemplars.

diff --git a/exporters/otlp/otlpmetric/otlpmetrichttp/internal/transform/metricdata.go b/exporters/otlp/otlpmetric/otlpmetrichttp/internal/transform/metricdata.go
@@ -279,10 +279,7 @@ func Temporality(t metricdata.Temporality) (mpb.AggregationTemporality, error) {
 // timeUnixNano on the zero Time returns 0.
 // The result does not depend on the location associated with t.
 func timeUnixNano(t time.Time) uint64 {
-	if t.IsZero() {
-		return 0
-	}
-	return uint64(t.UnixNano())
+	return uint64(max(0, t.UnixNano())) // nolint:gosec // Overflow checked.
 }
 
 // Exemplars returns a slice of OTLP Exemplars generated from exemplars.

diff --git a/exporters/otlp/otlptrace/internal/tracetransform/span.go b/exporters/otlp/otlptrace/internal/tracetransform/span.go
@@ -97,8 +97,8 @@ func span(sd tracesdk.ReadOnlySpan) *tracepb.Span {
 		SpanId:                 sid[:],
 		TraceState:             sd.SpanContext().TraceState().String(),
 		Status:                 status(sd.Status().Code, sd.Status().Description),
-		StartTimeUnixNano:      uint64(sd.StartTime().UnixNano()),
-		EndTimeUnixNano:        uint64(sd.EndTime().UnixNano()),
+		StartTimeUnixNano:      uint64(max(0, sd.StartTime().UnixNano())), // nolint:gosec // Overflow checked.
+		EndTimeUnixNano:        uint64(max(0, sd.EndTime().UnixNano())),   // nolint:gosec // Overflow checked.
 		Links:                  links(sd.Links()),
 		Kind:                   spanKind(sd.SpanKind()),
 		Name:                   sd.Name(),
@@ -178,7 +178,7 @@ func buildSpanFlags(sc trace.SpanContext) uint32 {
 		flags |= tracepb.SpanFlags_SPAN_FLAGS_CONTEXT_IS_REMOTE_MASK
 	}
 
-	return uint32(flags)
+	return uint32(flags) // nolint:gosec // Flags is a bitmask and can't be negative
 }
 
 // spanEvents transforms span Events to an OTLP span events.
@@ -192,7 +192,7 @@ func spanEvents(es []tracesdk.Event) []*tracepb.Span_Event {
 	for i := 0; i < len(es); i++ {
 		events[i] = &tracepb.Span_Event{
 			Name:                   es[i].Name,
-			TimeUnixNano:           uint64(es[i].Time.UnixNano()),
+			TimeUnixNano:           uint64(max(0, es[i].Time.UnixNano())), // nolint:gosec // Overflow checked.
 			Attributes:             KeyValues(es[i].Attributes),
 			DroppedAttributesCount: clampUint32(es[i].DroppedAttributeCount),
 		}

diff --git a/exporters/otlp/otlptrace/internal/tracetransform/span_test.go b/exporters/otlp/otlptrace/internal/tracetransform/span_test.go
@@ -68,6 +68,7 @@ func TestEmptySpanEvent(t *testing.T) {
 func TestSpanEvent(t *testing.T) {
 	attrs := []attribute.KeyValue{attribute.Int("one", 1), attribute.Int("two", 2)}
 	eventTime := time.Date(2020, 5, 20, 0, 0, 0, 0, time.UTC)
+	negativeEventTime := time.Date(1969, 7, 20, 20, 17, 0, 0, time.UTC)
 	got := spanEvents([]tracesdk.Event{
 		{
 			Name:       "test 1",
@@ -80,14 +81,21 @@ func TestSpanEvent(t *testing.T) {
 			Time:                  eventTime,
 			DroppedAttributeCount: 2,
 		},
+		{
+			Name:                  "test 3",
+			Attributes:            attrs,
+			Time:                  negativeEventTime,
+			DroppedAttributeCount: 2,
+		},
 	})
-	if !assert.Len(t, got, 2) {
+	if !assert.Len(t, got, 3) {
 		return
 	}
 	eventTimestamp := uint64(1589932800 * 1e9)
 	assert.Equal(t, &tracepb.Span_Event{Name: "test 1", Attributes: nil, TimeUnixNano: eventTimestamp}, got[0])
 	// Do not test Attributes directly, just that the return value goes to the correct field.
 	assert.Equal(t, &tracepb.Span_Event{Name: "test 2", Attributes: KeyValues(attrs), TimeUnixNano: eventTimestamp, DroppedAttributesCount: 2}, got[1])
+	assert.Equal(t, &tracepb.Span_Event{Name: "test 3", Attributes: KeyValues(attrs), TimeUnixNano: 0, DroppedAttributesCount: 2}, got[2])
 }
 
 func TestNilLinks(t *testing.T) {

diff --git a/exporters/zipkin/internal/internaltest/text_map_propagator.go b/exporters/zipkin/internal/internaltest/text_map_propagator.go
@@ -75,8 +75,8 @@ func (p *TextMapPropagator) Inject(ctx context.Context, carrier propagation.Text
 }
 
 // InjectedN tests if p has made n injections to carrier.
-func (p *TextMapPropagator) InjectedN(t *testing.T, carrier *TextMapCarrier, n int) bool {
-	if actual := p.stateFromCarrier(carrier).Injections; actual != uint64(n) {
+func (p *TextMapPropagator) InjectedN(t *testing.T, carrier *TextMapCarrier, n uint64) bool {
+	if actual := p.stateFromCarrier(carrier).Injections; actual != n {
 		t.Errorf("TextMapPropagator{%q} injected %d times, not %d", p.name, actual, n)
 		return false
 	}

diff --git a/internal/internaltest/text_map_propagator.go b/internal/internaltest/text_map_propagator.go
@@ -75,8 +75,8 @@ func (p *TextMapPropagator) Inject(ctx context.Context, carrier propagation.Text
 }
 
 // InjectedN tests if p has made n injections to carrier.
-func (p *TextMapPropagator) InjectedN(t *testing.T, carrier *TextMapCarrier, n int) bool {
-	if actual := p.stateFromCarrier(carrier).Injections; actual != uint64(n) {
+func (p *TextMapPropagator) InjectedN(t *testing.T, carrier *TextMapCarrier, n uint64) bool {
+	if actual := p.stateFromCarrier(carrier).Injections; actual != n {
 		t.Errorf("TextMapPropagator{%q} injected %d times, not %d", p.name, actual, n)
 		return false
 	}

diff --git a/internal/rawhelpers.go b/internal/rawhelpers.go
@@ -20,7 +20,8 @@ func RawToBool(r uint64) bool {
 }
 
 func Int64ToRaw(i int64) uint64 {
-	return uint64(i)
+	// Assumes original was a valid int64 (overflow not checked).
+	return uint64(i) // nolint: gosec
 }
 
 func RawToInt64(r uint64) int64 {

diff --git a/internal/shared/internaltest/text_map_propagator.go.tmpl b/internal/shared/internaltest/text_map_propagator.go.tmpl
@@ -75,8 +75,8 @@ func (p *TextMapPropagator) Inject(ctx context.Context, carrier propagation.Text
 }
 
 // InjectedN tests if p has made n injections to carrier.
-func (p *TextMapPropagator) InjectedN(t *testing.T, carrier *TextMapCarrier, n int) bool {
-	if actual := p.stateFromCarrier(carrier).Injections; actual != uint64(n) {
+func (p *TextMapPropagator) InjectedN(t *testing.T, carrier *TextMapCarrier, n uint64) bool {
+	if actual := p.stateFromCarrier(carrier).Injections; actual != n {
 		t.Errorf("TextMapPropagator{%q} injected %d times, not %d", p.name, actual, n)
 		return false
 	}

diff --git a/internal/shared/otlp/otlplog/transform/log.go.tmpl b/internal/shared/otlp/otlplog/transform/log.go.tmpl
@@ -139,10 +139,11 @@ func LogRecord(record log.Record) *lpb.LogRecord {
 // year 1678 or after 2262). timeUnixNano on the zero Time returns 0. The
 // result does not depend on the location associated with t.
 func timeUnixNano(t time.Time) uint64 {
-	if t.IsZero() {
+	nano := t.UnixNano()
+	if nano < 0 {
 		return 0
 	}
-	return uint64(t.UnixNano())
+	return uint64(nano) // nolint:gosec // Overflow checked.
 }
 
 // AttrIter transforms an [attribute.Iterator] into OTLP key-values.