Skip to content

Commit e4324fb

Browse files
committed
[pdata/pprofile] MergeTo: reserve index 0 of empty destination dictionary tables
Motivation: Profiles.MergeTo remaps entries from the source dictionary into the destination dictionary but never reserves index 0 of each destination table for that table's zero value. profiles.proto requires that "the element at index 0 MUST be the zero value for the dictionary's element type" (e.g. "" for string_table), since every unset reference (e.g. an unset Strindex) resolves to it. When the destination dictionary starts out empty, the first entry switched over lands at index 0 instead of the zero value, producing a non-conformant dictionary in which unset references silently resolve to real data. This is a data-correctness bug, not a crash: MergeTo itself does not panic or error. Concretely, if the first string merged into an empty destination is some real value (e.g. a semconv attribute key), any function/location that had an unset filename/name (Strindex == 0) will report that unrelated string as its filename/name after the merge, as described in the issue's pprofreceiver-based repro. Destinations that are already pre-populated with conformant zero values before calling MergeTo (the existing convention used throughout this package's own tests) are unaffected, since the fix only seeds tables that are still empty. Approach: Add reserveDictionaryZeroValues, called at the top of MergeTo (after the self-merge no-op check, before switchDictionary runs). It seeds index 0 of each of the destination dictionary's 7 tables (string, attribute, function, link, location, mapping, stack) with that table's zero value, but only for tables that are still empty. A table that already holds entries is left untouched, since inserting at index 0 would shift every existing index and invalidate references already pointing into it. This matches the fix suggested in the issue. Validation: Added TestProfilesMergeTo_ReservesDictionaryZeroValues, adapted from the issue's reproduction: it merges a conformant source (whose index 0 in every table is already the zero value, plus one real string at index 1) into a freshly empty destination, and asserts every destination table's index-0 entry is the zero value while index 1 still holds the real value. Confirmed the test fails (wrong value, then an index-out-of-range panic) when profiles_merge.go's fix is reverted, and passes with it applied. Ran: cd pdata/pprofile && go build ./... && go vet ./... && go test ./... All packages pass, including the existing MergeTo test suite in profiles_merge_test.go, which already covers destinations that are pre-populated with zero values by the caller and confirms no regression there. Fixes #15661 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
1 parent 52e6bf4 commit e4324fb

3 files changed

Lines changed: 116 additions & 0 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: bug_fix
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. receiver/otlp)
7+
component: pkg/pprofile
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: "`Profiles.MergeTo` no longer overwrites index 0 of an empty destination dictionary table with real data"
11+
12+
# One or more tracking issues or pull requests related to the change
13+
issues: [15661]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext: |
19+
`MergeTo` did not reserve index 0 of the destination dictionary's tables for their zero value before
20+
remapping entries into them. When the destination dictionary was empty, the first entry merged in would
21+
land at index 0, the slot every unset reference (e.g. an unset Strindex) resolves to, producing a
22+
dictionary that violates the `ProfilesDictionary` protocol. `MergeTo` now seeds index 0 of any table that
23+
is still empty with that table's zero value before remapping.
24+
25+
# Optional: The change log or logs in which this entry should be included.
26+
# e.g. '[user]' or '[user, api]'
27+
# Include 'user' if the change is relevant to end users.
28+
# Include 'api' if there is a change to a library API.
29+
# Default: '[user]'
30+
change_logs: [api]

pdata/pprofile/profiles_merge.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@ func (ms Profiles) MergeTo(dest Profiles) error {
1313
return nil
1414
}
1515

16+
reserveDictionaryZeroValues(dest.Dictionary())
17+
1618
if err := ms.switchDictionary(ms.Dictionary(), dest.Dictionary()); err != nil {
1719
return err
1820
}
@@ -22,3 +24,34 @@ func (ms Profiles) MergeTo(dest Profiles) error {
2224

2325
return nil
2426
}
27+
28+
// reserveDictionaryZeroValues seeds index 0 of each of dst's tables with that
29+
// table's zero value, but only for tables that are still empty. Per the
30+
// ProfilesDictionary contract, index 0 of every table MUST hold the zero
31+
// value for that table's element type, since unset references (e.g. an
32+
// unset Strindex) resolve to it. Tables that already hold entries are left
33+
// untouched, since inserting at index 0 would invalidate every existing
34+
// reference into them.
35+
func reserveDictionaryZeroValues(dst ProfilesDictionary) {
36+
if dst.StringTable().Len() == 0 {
37+
dst.StringTable().Append("")
38+
}
39+
if dst.AttributeTable().Len() == 0 {
40+
dst.AttributeTable().AppendEmpty()
41+
}
42+
if dst.FunctionTable().Len() == 0 {
43+
dst.FunctionTable().AppendEmpty()
44+
}
45+
if dst.LinkTable().Len() == 0 {
46+
dst.LinkTable().AppendEmpty()
47+
}
48+
if dst.LocationTable().Len() == 0 {
49+
dst.LocationTable().AppendEmpty()
50+
}
51+
if dst.MappingTable().Len() == 0 {
52+
dst.MappingTable().AppendEmpty()
53+
}
54+
if dst.StackTable().Len() == 0 {
55+
dst.StackTable().AppendEmpty()
56+
}
57+
}

pdata/pprofile/profiles_merge_test.go

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -585,6 +585,59 @@ func TestProfilesMergeTo(t *testing.T) {
585585
}
586586
}
587587

588+
// TestProfilesMergeTo_ReservesDictionaryZeroValues verifies that MergeTo
589+
// reserves index 0 of every destination table for that table's zero value,
590+
// even when the destination dictionary starts out completely empty (not
591+
// pre-populated by the caller). See
592+
// https://github.com/open-telemetry/opentelemetry-collector/issues/15661.
593+
func TestProfilesMergeTo_ReservesDictionaryZeroValues(t *testing.T) {
594+
src := NewProfiles()
595+
dic := src.Dictionary()
596+
dic.StringTable().Append("") // conformant source
597+
dic.AttributeTable().AppendEmpty()
598+
dic.StackTable().AppendEmpty()
599+
dic.LocationTable().AppendEmpty()
600+
dic.FunctionTable().AppendEmpty()
601+
dic.MappingTable().AppendEmpty()
602+
dic.LinkTable().AppendEmpty()
603+
dic.StringTable().Append("inuse_space") // index 1
604+
605+
prof := src.ResourceProfiles().AppendEmpty().ScopeProfiles().AppendEmpty().Profiles().AppendEmpty()
606+
prof.SampleType().SetTypeStrindex(1)
607+
608+
dest := NewProfiles()
609+
require.NoError(t, src.MergeTo(dest))
610+
611+
assert.Equal(t, "", dest.Dictionary().StringTable().At(0))
612+
assert.Equal(t, "inuse_space", dest.Dictionary().StringTable().At(1))
613+
614+
require.Equal(t, 1, dest.Dictionary().AttributeTable().Len())
615+
attr := dest.Dictionary().AttributeTable().At(0)
616+
assert.Zero(t, attr.KeyStrindex())
617+
assert.Zero(t, attr.UnitStrindex())
618+
assert.Equal(t, pcommon.ValueTypeEmpty, attr.Value().Type())
619+
620+
require.Equal(t, 1, dest.Dictionary().StackTable().Len())
621+
assert.Zero(t, dest.Dictionary().StackTable().At(0).LocationIndices().Len())
622+
623+
require.Equal(t, 1, dest.Dictionary().LocationTable().Len())
624+
loc := dest.Dictionary().LocationTable().At(0)
625+
assert.Zero(t, loc.MappingIndex())
626+
assert.Zero(t, loc.Address())
627+
assert.Zero(t, loc.Lines().Len())
628+
629+
require.Equal(t, 1, dest.Dictionary().FunctionTable().Len())
630+
fn := dest.Dictionary().FunctionTable().At(0)
631+
assert.Zero(t, fn.NameStrindex())
632+
assert.Zero(t, fn.SystemNameStrindex())
633+
assert.Zero(t, fn.FilenameStrindex())
634+
635+
require.Equal(t, 1, dest.Dictionary().MappingTable().Len())
636+
assert.Zero(t, dest.Dictionary().MappingTable().At(0).FilenameStrindex())
637+
638+
require.Equal(t, 1, dest.Dictionary().LinkTable().Len())
639+
}
640+
588641
func TestProfilesMergeToSelf(t *testing.T) {
589642
profiles := NewProfiles()
590643
profiles.Dictionary().StringTable().Append("", "test")

0 commit comments

Comments
 (0)