Conversation
CheckedEntityCast picked dynamic_cast only under _CPPRTTI, which only MSVC defines. GCC and Clang builds with RTTI on fell through to the kTypeName id check, so a downstream Linux server stopped compiling on entity types that do not declare kTypeName. Also accept __GXX_RTTI.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. Walkthrough
ChangesEntity casting
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to RTTI-enabled GCC/Clang builds use polymorphic entity casts, while RTTI-disabled builds retain the registered-type-ID fallback. The inspected callers are compatible, and no actionable merge risk is established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to On GCC and Clang builds with RTTI enabled, typed entity lookups can now accept subclasses where the previous fallback required an exact registered type. No authorization bypass is demonstrated, but the effect on external consumers is unknown. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks each cast with care Comment |
|
Addressed with alternative approach downstream. |
Problem
CheckedEntityCast<T>(added in 75263da) usesdynamic_castonly under#if defined(_CPPRTTI). MSVC is the only compiler that defines_CPPRTTI. GCC and Clang signal RTTI with__GXX_RTTI, so on Linux every build falls through to the/GR-fallback, even with RTTI on. That fallback needsT::kTypeName, and any typed wrapper instantiated on a type without one fails to compile.ReplicationManager::GetEntity<T>,GetViewerAs<T>,ForEach<T>andEntityCollectionall go through this cast.HogwartsMP's Linux server hits this: its entity types register by name constants and don't declare
kTypeName.Failed HogwartsMP build: https://github.com/hogwarts-mp/mod/actions/runs/36290998653/job/108540951455
Windows builds were unaffected. The framework's own CI only instantiates the cast on
TextLabelEntity, which does declarekTypeName, so it didn't catch this.Change
#if defined(_CPPRTTI) || defined(__GXX_RTTI)MSVC behaviour doesn't change. GCC and Clang with RTTI on now use
dynamic_castlike MSVC does. Builds with/GR-or-fno-rttistill use the type-id check.Testing
Not compiled locally on Linux (no GCC toolchain here). GCC and Clang define
__GXX_RTTIwhenever RTTI is on and leave it undefined under-fno-rtti.