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<CPerPartitionCategoryIdMapper>(*this) (default copy of m_Mappings).
map() constructs CGlobalCategoryIds 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:
- 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).
- Store the categorizer key in
CGlobalCategoryId by shared ownership (e.g. std::shared_ptr<const std::string>) 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.
Summary
CGlobalCategoryIdstores a raw pointer to the owning mapper's categorizer-key string (m_CategorizerKey = &categorizerKey). BecauseCPerPartitionCategoryIdMapper::clone()performs a default (shallow) copy of itsm_Mappingsvector, the clonedCGlobalCategoryIdentries 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 storesm_CategorizerKey = &categorizerKey.lib/api/CPerPartitionCategoryIdMapper.ccclone()→std::make_shared<CPerPartitionCategoryIdMapper>(*this)(default copy ofm_Mappings).map()constructsCGlobalCategoryIds bound tom_CategorizerKey.Impact
CPerPartitionCategoryIdMapper::acceptPersistInserteronly readsglobalCategoryId.globalId(); it never dereferences the key back-pointer.CGlobalCategoryId::print()/operator<<(i.e. logging/diagnostics) on a cloned mapper whose original has since been destroyed. In practiceclone()is used for background persistence snapshots, so the window is narrow — but it is undefined behaviour.Suggested fix
Make
CGlobalCategoryIdnot depend on a pointer whose lifetime it doesn't control. Options:m_CategorizerKey(e.g. a user-provided copy constructor inCPerPartitionCategoryIdMapperthat rewrites each mapping's key reference).CGlobalCategoryIdby shared ownership (e.g.std::shared_ptr<const std::string>) so clones stay valid independent of the original mapper.Option 1 keeps
CGlobalCategoryIdlightweight; 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.