elastic / elastic/ml-cpp

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

Offen
#3,144 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
Vorherrschende Sprache
C++
Sterne
157
Forks
67
Ø Merge
17 Std. 52 Min.
Gemergte PRs (30 T.)
20

Beschreibung

### 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.

Beitragsleitfaden

Beitragsleitfaden öffnen

Rechercherichtung

Lesen Sie lib/api/CGlobalCategoryId.cc und lib/api/CPerPartitionCategoryIdMapper.cc, wobei Sie sich auf den Konstruktor, clone(), map() und acceptPersistInserter() konzentrieren. Verfolgen Sie, wie geklonte Mappings den categorizer-key pointer beibehalten, und prüfen Sie anschließend vorhandene Copy- und Persistenztests auf ihre Abdeckung. Als erledigt gilt die Aufgabe, wenn ein Clone noch sicher ausgegeben werden kann, nachdem der ursprüngliche Mapper zerstört wurde, ohne das Persistenzverhalten zu ändern.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
cpp
Bereich
backend
Issue-Typ
Bug
Schwierigkeit
4/5
Geschätzter Aufwand
3-5 Tage
Aktivitätsstatus
Ruhig
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
55/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.