fix(rpc4): NUL-terminate RPC4GlobalRegistration functionName - #52
Conversation
The copy loops in all four RPC4GlobalRegistration constructors never wrote a terminating NUL, so a uniqueID of exactly RPC4_GLOBAL_REGISTRATION_MAX_FUNCTION_NAME_LENGTH bytes filled the buffer and the later strlen in RakString::Assign (via RPC4::OnAttach -> RegisterFunction/RegisterSlot/RegisterBlockingFunction) read past the array. Clamp the copy to what fits (rather than only asserting, which is a no-op in release and would still let an over-long name overflow the array) and always write the terminator. RakAssert still flags truncation in debug. Fixes #6
|
Warning Review limit reached
Next review available in: 33 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
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. Comment |
The truncation-backstop test guards itself with #ifdef _DEBUG to skip when RakAssert is live, but _DEBUG was a PRIVATE compile definition on the library target only — the test TU never saw it, so in Debug builds the test ran anyway and tripped the assert. Mirror the library's Debug definition onto the UnitTests target so the guard matches the library's actual assert configuration.
Fixes #6.
Problem
All four
RPC4GlobalRegistrationconstructors copieduniqueIDinto the fixedGlobalRegistration::functionName[48]buffer without writing a terminating NUL. AuniqueIDof exactlyRPC4_GLOBAL_REGISTRATION_MAX_FUNCTION_NAME_LENGTHcharacters filled the buffer completely, andRPC4::OnAttach()later handed it toRegisterFunction/RegisterSlot/RegisterBlockingFunction, whereRakString::Assigncallsstrlen— reading past the array.Fix
Slightly stronger than the fix sketched in the issue (which would still have written the terminator one past the end for a name longer than the buffer):
RPC4_GLOBAL_REGISTRATION_MAX_FUNCTION_NAME_LENGTH - 1characters, so even an over-long name cannot overflow the array in release builds whereRakAssertcompiles out.RakAssert(uniqueID[i]==0)still flags any truncation in debug builds.Applied identically to all four constructors.
Tests
New
Tests/Unit/RPC4GlobalRegistrationTests.cpp:MaxLengthNameIsStoredNulTerminated— a 47-char name round-trips through global registration →OnAttach→UnregisterFunction.OverlongNameIsTruncatedNotOverflowed— a 53-char name is clamped to 47 chars + NUL instead of overflowing (release-only; skipped in debug where the assert fires).Full unit suite (121 tests) and the RPC4 integration test pass on a Release build.