Fix stale goroutine label, validate UTF-8 on label strings

.github/workflows/unit-test-on-pull-request.yml

@@ -152,7 +152,6 @@ jobs:
           # https://github.com/cilium/ci-kernels/pkgs/container/ci-kernels/versions?filters%5Bversion_type%5D=tagged
 
           # AMD64
-          - { target_arch: amd64, kernel: 5.4.276 }
           - { target_arch: amd64, kernel: 5.10.217 }
           - { target_arch: amd64, kernel: 5.15.159 }
           - { target_arch: amd64, kernel: 6.1.91 }
@@ -281,7 +280,6 @@ jobs:
     strategy:
       matrix:
         include:
-          - { target_arch: amd64, kernel: 5.4.276 }
           - { target_arch: amd64, kernel: 5.10.217 }
           - { target_arch: amd64, kernel: 5.15.159 }
           - { target_arch: amd64, kernel: 6.1.91 }

metrics/metrics.json

@@ -2222,6 +2222,20 @@
     "name": "UnwindLuaJITErrLMismatch",
     "field": "bpf.luajit.errors.l_mismatch",
     "id": 304
+  },
+  {
+    "description": "Number of Go custom labels dropped because the label name was empty or not valid UTF-8",
+    "type": "counter",
+    "name": "GoLabelsDroppedInvalidName",
+    "field": "agent.golabels.dropped.invalid_name",
+    "id": 305
+  },
+  {
+    "description": "Number of Go custom labels dropped because the label value was not valid UTF-8",
+    "type": "counter",
+    "name": "GoLabelsDroppedInvalidValue",
+    "field": "agent.golabels.dropped.invalid_value",
+    "id": 306
   }
 
 ]

support/ebpf/go_labels.ebpf.c

@@ -38,19 +38,27 @@ get_go_custom_labels_from_slice(PerCPURecord *record, void *labels_slice_ptr)
     if (i >= labels_slice.len)
       break;
     CustomLabel *lbl = &out->labels[i];
+    // The PerCPURecord is reused across traces and bpf_probe_read_user() only
+    // writes the bytes it reads, so a shorter key/value would otherwise inherit
+    // trailing bytes from a previous trace. Userspace scans for the first NUL,
+    // so a single terminator after the bytes we read is sufficient. The key/val
+    // buffers are CUSTOM_LABEL_MAX_*_LEN + 1, so position klen/vlen is always
+    // in-bounds for the terminator.
     u8 klen          = MIN(record->labels[i * 2].len, CUSTOM_LABEL_MAX_KEY_LEN);
     if (bpf_probe_read_user(lbl->key, klen, record->labels[i * 2].str)) {
       DEBUG_PRINT(
         "cl: failed to read key for custom label (%lx)", (unsigned long)record->labels[i * 2].str);
       return false;
     }
-    u8 vlen = MIN(record->labels[i * 2 + 1].len, CUSTOM_LABEL_MAX_VAL_LEN);
+    lbl->key[klen] = 0;
+    u8 vlen        = MIN(record->labels[i * 2 + 1].len, CUSTOM_LABEL_MAX_VAL_LEN);
     if (bpf_probe_read_user(lbl->val, vlen, record->labels[i * 2 + 1].str)) {
       DEBUG_PRINT(
         "cl: failed to read key for custom label (%lx)",
         (unsigned long)record->labels[i * 2 + 1].str);
       return false;
     }
+    lbl->val[vlen] = 0;
   }
   out->len = num_to_read;
 
@@ -118,14 +126,20 @@ get_go_custom_labels_from_map(PerCPURecord *record, void *labels_map_ptr_ptr, Go
       char *vstr       = map_value->values[i].str;
       unsigned vlen    = map_value->values[i].len;
       if (tophash != 0 && kstr != NULL) {
-        if (bpf_probe_read_user(lbl->key, MIN(klen, CUSTOM_LABEL_MAX_KEY_LEN), kstr)) {
+        // Terminate the bytes we read with a NUL; see get_go_custom_labels_from_slice
+        // for why this is required (the PerCPURecord is reused across traces).
+        u8 klen_clamped = MIN(klen, CUSTOM_LABEL_MAX_KEY_LEN);
+        if (bpf_probe_read_user(lbl->key, klen_clamped, kstr)) {
           DEBUG_PRINT("cl: failed to read key for custom label (%lx)", (unsigned long)kstr);
           return false;
         }
-        if (bpf_probe_read_user(lbl->val, MIN(vlen, CUSTOM_LABEL_MAX_VAL_LEN), vstr)) {
+        lbl->key[klen_clamped] = 0;
+        u8 vlen_clamped        = MIN(vlen, CUSTOM_LABEL_MAX_VAL_LEN);
+        if (bpf_probe_read_user(lbl->val, vlen_clamped, vstr)) {
           DEBUG_PRINT("cl: failed to read value for custom label");
           return false;
         }
+        lbl->val[vlen_clamped] = 0;
         out->len++;
       }
     }

tracer/string_test.go

@@ -0,0 +1,162 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package tracer
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestGoString(t *testing.T) {
+	tests := map[string]struct {
+		input     []byte
+		wantValue string
+	}{
+		"plain ascii": {
+			input:     []byte("tenant\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"),
+			wantValue: "tenant",
+		},
+		"valid multi-byte utf8": {
+			input:     append([]byte("héllo"), 0),
+			wantValue: "héllo",
+		},
+		"empty buffer": {
+			input:     make([]byte, 16),
+			wantValue: "",
+		},
+		"no nul terminator uses whole buffer": {
+			input:     []byte("exactlysixteenb!"),
+			wantValue: "exactlysixteenb!",
+		},
+		"stale bytes after nul are discarded": {
+			// Models a short string written into a per-CPU slot that previously
+			// held a longer one. Everything past the first NUL must be dropped.
+			input:     []byte("tier\x00equest-trace"),
+			wantValue: "tier",
+		},
+		"invalid utf8 is passed through unvalidated": {
+			// goString is used for comm, which is kernel-supplied and trusted
+			// as-is; validation happens only for label strings.
+			input:     []byte{'b', 'a', 'd', 0x80, 0x00},
+			wantValue: "bad\x80",
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			got := goString(tc.input)
+			require.Equal(t, tc.wantValue, got.String())
+		})
+	}
+}
+
+func TestGoLabelKey(t *testing.T) {
+	tests := map[string]struct {
+		input     []byte
+		wantValue string
+		wantOK    bool
+	}{
+		"plain ascii": {
+			input:     []byte("tenant\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"),
+			wantValue: "tenant",
+			wantOK:    true,
+		},
+		"valid multi-byte utf8": {
+			input:     append([]byte("héllo"), 0),
+			wantValue: "héllo",
+			wantOK:    true,
+		},
+		"empty buffer drops": {
+			// An empty key cannot be grouped against, so reject.
+			input:  make([]byte, 16),
+			wantOK: false,
+		},
+		"stale bytes after nul are discarded": {
+			input:     []byte("tier\x00equest-trace"),
+			wantValue: "tier",
+			wantOK:    true,
+		},
+		"mid-rune truncation drops the whole key": {
+			// Keys are strict: a salvageable value-style prefix is not enough,
+			// since dropping the trailing byte would silently change which key
+			// samples are grouped under.
+			input:  []byte{'o', 'k', 0xE2, 0x00},
+			wantOK: false,
+		},
+		"trailing lone continuation byte drops": {
+			input:  []byte{'a', 'b', 'c', 0x80, 0x00},
+			wantOK: false,
+		},
+		"all-invalid bytes drop": {
+			input:  []byte{0x80, 0x80, 0x00},
+			wantOK: false,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			got, ok := goLabelKey(tc.input)
+			require.Equal(t, tc.wantOK, ok)
+			require.Equal(t, tc.wantValue, got.String())
+		})
+	}
+}
+
+func TestGoLabelValue(t *testing.T) {
+	tests := map[string]struct {
+		input     []byte
+		wantValue string
+		wantOK    bool
+	}{
+		"plain ascii": {
+			input:     []byte("tenant\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"),
+			wantValue: "tenant",
+			wantOK:    true,
+		},
+		"valid multi-byte utf8": {
+			input:     append([]byte("héllo"), 0),
+			wantValue: "héllo",
+			wantOK:    true,
+		},
+		"empty buffer is valid": {
+			input:     make([]byte, 16),
+			wantValue: "",
+			wantOK:    true,
+		},
+		"stale bytes after nul are discarded": {
+			input:     []byte("tier\x00equest-trace"),
+			wantValue: "tier",
+			wantOK:    true,
+		},
+		"mid-rune truncation salvages valid prefix": {
+			// 3-byte rune (0xE2 0x98 0x83 = U+2603) cut after the first byte.
+			// The valid "ok" prefix must be preserved.
+			input:     []byte{'o', 'k', 0xE2, 0x00},
+			wantValue: "ok",
+			wantOK:    true,
+		},
+		"trailing lone continuation byte salvages valid prefix": {
+			input:     []byte{'a', 'b', 'c', 0x80, 0x00},
+			wantValue: "abc",
+			wantOK:    true,
+		},
+		"all-invalid bytes drop": {
+			input:  []byte{0x80, 0x80, 0x00},
+			wantOK: false,
+		},
+		"single invalid byte drops": {
+			input:  []byte{0xC0, 0x00},
+			wantOK: false,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			got, ok := goLabelValue(tc.input)
+			require.Equal(t, tc.wantOK, ok)
+			require.Equal(t, tc.wantValue, got.String())
+		})
+	}
+}