Skip to content

bug: data providers hand out aliased internal maps from GetData (shallow/no copy) #159

Description

@robbyt

Summary

The provider read path (GetData) returns references to internal storage, so a caller or engine that mutates the returned map corrupts the provider's data and can race with concurrent readers. This undermines the concurrency guarantee the write path (AddDataToContext) maintains.

1. ContextProvider.GetData returns the live stored map

platform/data/contextProvider.go:44:

d, ok := value.(map[string]any)
...
return d, nil   // the exact map stored in the context, no copy

AddDataToContext deep-copies at every level to guarantee derived-context independence (see the # Concurrency note at contextProvider.go:53-61), but GetData hands out the shared storage. A caller mutation corrupts the parent context, all sibling derived chains, and all future GetData results, and races under -race with a concurrent AddDataToContext copy or another GetData.

Repro (confirmed): goroutine A runs d, _ := p.GetData(ctx); d["k"] = "hacked" while goroutine B evaluates with the same ctx → B observes A's mutation; under -race the pair is a data race.

Fix: return deepCopyMap(d), nil.

2. StaticProvider.GetData shallow clone contradicts its doc

platform/data/staticProvider.go:32-35:

// GetData returns the static data map, cloned to prevent modification.
func (p *StaticProvider) GetData(_ context.Context) (map[string]any, error) {
	return maps.Clone(p.data), nil   // shallow; nested maps/slices shared
}

maps.Clone is shallow, so nested maps and slices remain shared with p.data, contradicting the "cloned to prevent modification" doc.

Repro (confirmed): d, _ := p.GetData(ctx); d["cfg"].(map[string]any)["x"] = 999 permanently corrupts the provider's static data; every later GetData/eval sees 999. Concurrent mutation plus GetData races on the shared inner map. CompositeProvider.GetData amplifies this: deepMerge stores dst-only nested maps by reference (compositeProvider.go:58), so the merged result aliases the static provider's internals.

Fix: use the existing deepCopyMap (contextProvider.go:187) instead of maps.Clone, and align the doc with the behavior.

Impact

Medium; high if any engine mutates its input map in place. The library advertises thread-safe data management, which the read path breaks for any consumer that touches the returned map.

Fix

Deep-copy on the read path in both providers (reuse deepCopyMap), and add -race tests that mutate the returned map and assert the provider's state is unchanged.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions