-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Session ID Allocation #16895
Fix Session ID Allocation #16895
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the whole, a really great PR, thank you for clean-ing this up! The ownership model is much better with the fact that you're allocating a session now from SessionManager, and not just a SessionID.
…ingSession object
Overloads make this superfluous.
Co-authored-by: Boris Zbarsky <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty solid change for session ID allocation implementation.
While reading the code, I feel there are a few improvements we can do in the future:
PairingSession
as a base class for PASE and CASE pairing, almost provides nothing in common for these 2 cases. We can leavePairingSession
an almost empty virtual interface and push its fields down intoPASESessoin
andCASESession
classes.- Every API of
PairingSession
takes an argument ofSessionManager
, since it is highly coupled with the SessionManager, we can add a ref to SessionManager into its member.
* Fix Session ID Allocation Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass * fix Werror=conversion * fix VerifyOrExit lint error * fix Werror=unused-but-set-variable with detail logging disabled * fix tv-casting-app build * pass SessionHolder by reference to reduce stack size * bypass -Wstack-usage= in TestPASESession to fix spurious stack warning * Update src/transport/SecureSession.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/transport/SecureSessionTable.h Co-authored-by: Boris Zbarsky <[email protected]> * per bzbarsky-apple, document that allocation of sessions to caller-specified IDs is for testing * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <[email protected]> * per mrjerryjohns, delegate allocation of secure sessions to base PairingSession object * remove use of Optional<uint16_t> session IDs Overloads make this superfluous. * init session ID randomly; add more checks for invalid 0 session ID * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <[email protected]> * per bzbarsky-apple, pass reference type to SessionHolderWithDelegate * restyle * per kghost, pass SessionHandle, not SessionHolder * make sure to grab the session in NewPairing * fixup doxy params * fix up comments * add a test case to verify the session ID allocator does not have collisions * reduce number of session ID allocator collision test iterations to fix CI timeout * fix loop sentinel in TestSessionManager * increase gcc_debug test phase timeout * increase gcc-debug total timeout to 65 minutes Co-authored-by: Boris Zbarsky <[email protected]>
* Fix Session ID Allocation Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass * fix Werror=conversion * fix VerifyOrExit lint error * fix Werror=unused-but-set-variable with detail logging disabled * fix tv-casting-app build * pass SessionHolder by reference to reduce stack size * bypass -Wstack-usage= in TestPASESession to fix spurious stack warning * Update src/transport/SecureSession.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/transport/SecureSessionTable.h Co-authored-by: Boris Zbarsky <[email protected]> * per bzbarsky-apple, document that allocation of sessions to caller-specified IDs is for testing * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <[email protected]> * per mrjerryjohns, delegate allocation of secure sessions to base PairingSession object * remove use of Optional<uint16_t> session IDs Overloads make this superfluous. * init session ID randomly; add more checks for invalid 0 session ID * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <[email protected]> * per bzbarsky-apple, pass reference type to SessionHolderWithDelegate * restyle * per kghost, pass SessionHandle, not SessionHolder * make sure to grab the session in NewPairing * fixup doxy params * fix up comments * add a test case to verify the session ID allocator does not have collisions * reduce number of session ID allocator collision test iterations to fix CI timeout * fix loop sentinel in TestSessionManager * increase gcc_debug test phase timeout * increase gcc-debug total timeout to 65 minutes Co-authored-by: Boris Zbarsky <[email protected]>
* Fix Session ID Allocation Secure session ID allocation currently suffers the following problems: * fragmentation has worst case behavior where there may be as few as 2 outstanding sessions, but the allocator will become full * there is no formal coupling to the session manager object, but yet there can only be one allocator per session manager; the current solution is for the allocator to share static state * IDs are proposed *to* the session manager, which means the session manager can only prevent collisions by failing on session creation or by evicting sessions; it currently does the latter * session ID allocation is manual, so leaks are likely This commit solves these problems by moving ID allocation into the session manager and by leveraging the session table itself as the source of truth for available IDs. Whereas the old flow was: * allocate session ID * initiate PASE or CASE * (on success) allocate session table entry The new flow is: * allocate session table entry with non-colliding ID * initiate PASE or CASE * activate the session in the table Allocation uses a next-session-ID clue, which is 1 more than the most recent allocation, and also searches the session table to make sure a non-colliding value is returned. Allocation time complexity is O(kMaxSessionCount^2/64) in the current implementation. Lifecycle of the pending session table entries also leverages the SessionHolder object to alleviate our need to manually free resources. Fixes: project-chip#7835, project-chip#12821 Testing: * Added an allocation test to TestSessionManager * All other code paths are heavily integrated into existing tests * All existing unit tests pass * fix Werror=conversion * fix VerifyOrExit lint error * fix Werror=unused-but-set-variable with detail logging disabled * fix tv-casting-app build * pass SessionHolder by reference to reduce stack size * bypass -Wstack-usage= in TestPASESession to fix spurious stack warning * Update src/transport/SecureSession.h Co-authored-by: Boris Zbarsky <[email protected]> * Update src/transport/SecureSessionTable.h Co-authored-by: Boris Zbarsky <[email protected]> * per bzbarsky-apple, document that allocation of sessions to caller-specified IDs is for testing * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <[email protected]> * per mrjerryjohns, delegate allocation of secure sessions to base PairingSession object * remove use of Optional<uint16_t> session IDs Overloads make this superfluous. * init session ID randomly; add more checks for invalid 0 session ID * Update src/transport/PairingSession.h Co-authored-by: Boris Zbarsky <[email protected]> * per bzbarsky-apple, pass reference type to SessionHolderWithDelegate * restyle * per kghost, pass SessionHandle, not SessionHolder * make sure to grab the session in NewPairing * fixup doxy params * fix up comments * add a test case to verify the session ID allocator does not have collisions * reduce number of session ID allocator collision test iterations to fix CI timeout * fix loop sentinel in TestSessionManager * increase gcc_debug test phase timeout * increase gcc-debug total timeout to 65 minutes Co-authored-by: Boris Zbarsky <[email protected]>
Secure session ID allocation currently suffers the following problems:
2 outstanding sessions, but the allocator will become full
yet there can only be one allocator per session manager; the
current solution is for the allocator to share static state
manager can only prevent collisions by failing on session creation
or by evicting sessions; it currently does the latter
This commit solves these problems by moving ID allocation into the
session manager and by leveraging the session table itself as the
source of truth for available IDs. Whereas the old flow was:
The new flow is:
Allocation uses a next-session-ID clue, which is 1 more than the most
recent allocation, and also searches the session table to make sure
a non-colliding value is returned. Allocation time complexity is
O(kMaxSessionCount^2/64) in the current implementation.
Lifecycle of the pending session table entries also leverages the
SessionHolder object to alleviate our need to manually free resources.
Fixes: #7835, #12821
Testing: