elastic / elastic/ml-cpp

CGlobalCategoryId holds a raw back-pointer to the mapper's categorizer key, which dangles after clone()

Open
#3,144 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
157
Forks
67
Avg merge
12h 48m
Merged PRs (30d)
16

Description

### Summary

`CGlobalCategoryId` stores a **raw pointer** to the owning mapper's categorizer-key string (`m_CategorizerKey = &categorizerKey`). Because `CPerPartitionCategoryIdMapper::clone()` performs a default (shallow) copy of its `m_Mappings` vector, the cloned `CGlobalCategoryId` entries still point at the **original** mapper's key string rather than the clone's. If the original mapper is destroyed while the clone is still alive, those pointers dangle.

This is a latent lifetime issue, distinct from the state-corruption crash class tracked in #2875. Splitting it out here so it isn't lost.

### Where

- `lib/api/CGlobalCategoryId.cc` — the `(int, const std::string&, CLocalCategoryId)` constructor stores `m_CategorizerKey = &categorizerKey`.
- `lib/api/CPerPartitionCategoryIdMapper.cc`
- `clone()` → `std::make_shared(*this)` (default copy of `m_Mappings`).
- `map()` constructs `CGlobalCategoryId`s bound to `m_CategorizerKey`.

### Impact

- **Persistence is unaffected**: `CPerPartitionCategoryIdMapper::acceptPersistInserter` only reads `globalCategoryId.globalId()`; it never dereferences the key back-pointer.
- The dangling read is only reachable via `CGlobalCategoryId::print()` / `operator<<` (i.e. logging/diagnostics) on a cloned mapper whose original has since been destroyed. In practice `clone()` is used for background persistence snapshots, so the window is narrow — but it is undefined behaviour.

### Suggested fix

Make `CGlobalCategoryId` not depend on a pointer whose lifetime it doesn't control. Options:

1. Have the copy path re-bind cloned mappings to the clone's own `m_CategorizerKey` (e.g. a user-provided copy constructor in `CPerPartitionCategoryIdMapper` that rewrites each mapping's key reference).
2. Store the categorizer key in `CGlobalCategoryId` by shared ownership (e.g. `std::shared_ptr`) so clones stay valid independent of the original mapper.

Option 1 keeps `CGlobalCategoryId` lightweight; option 2 is more robust but adds an allocation per key.

### Notes

Found during the deep dive accompanying the #2875 restore-hardening work (PR #3143). No crash has been attributed to this yet; filing pre-emptively.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.