Refactor SessionHandle: use variant for secure&unauthenticated, secure session only stores local session id in the handle. #11266
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
e04d743 to
1036a8e
Compare
|
PR #11266: Size comparison from 1073bb2 to 1036a8e Increases (21 builds for esp32, k32w, mbed, nrfconnect, p6, qpg)
Decreases (26 builds for efr32, esp32, k32w, mbed, nrfconnect, p6, qpg, telink)
Full report (29 builds for efr32, esp32, k32w, mbed, nrfconnect, p6, qpg, telink)
|
bzbarsky-apple
requested changes
Nov 1, 2021
| Transport::SecureSession * SessionHandle::AsSecureSession() const | ||
| { | ||
| const VariantSecureSession & session = mSession.Get<VariantSecureSession>(); | ||
| return session.mSessionManager.GetSecureSession(session.mLocalSessionId); |
There was a problem hiding this comment.
So this can still return null, right? But some of the callsites that use this (possibly indirectly now that it's hidden under operator-> had their null-checks removed? Why?
| /* | ||
| * @brief A handle to all types of sessions | ||
| * | ||
| * A SessionHandle always referring to a valid session, it will never dangling. |
There was a problem hiding this comment.
I don't think this is true given the mSessionManager.GetSecureSession(session.mLocalSessionId); bit. This won't dangle, but it can return null, not a session.
There was a problem hiding this comment.
I suppose the assertion is that given correct code, we expect the session handle to refer to a session that will have existed at one time. It can still definitely be the case that the session has since been evicted.
All of this makes me a bit nervous. Correct operation here is predicated on the session ID space not wrapping over dangling session handles. But you can't actually guarantee this with the session ID allocator because nothing in the system can cleanup outstanding session handles when underlying sessions are evicted.
In short, while hiding the underlying session pointers prevents dereference of an invalid pointer, it does NOT prevent this session ID wrap problem. I bet I could devise a pathological test case where a session is evicted, 65535 subsequent sessions are opened and then closed, one more session is opened and then a dangling session handle from the first, evicted session becomes an incorrect match to the newest session.
To be clear, this isn't a new problem with this PR. This PR just makes this more obvious by stripping everything but the session ID out of the session handle.
I don't know how this can be fixed unless the session manager can reach into the system and explicitly invalidate outstanding session handles. The most obvious design would change the session handles into pointers to actual, allocated memory in the session manager.
But we also need to be able to forcibly evict sessions. This implies a need to be able to execute a failure callback chain for any session that's being forcibly evicted. This might be possible with addition of a CancelExchange(session ID) interface for the exchange manager.
Complicated, but probably doable...
There was a problem hiding this comment.
Can we perhaps do something similar to file descriptors? I think it could be workable for the session manager to forcibly close sessions (as I think it does now), but only for the holders of session handles to release session IDs for reuse by the session ID allocator.
| /* | ||
| * @brief A handle to all types of sessions | ||
| * | ||
| * A SessionHandle always referring to a valid session, it will never dangling. |
There was a problem hiding this comment.
Suggested change
| * A SessionHandle always referring to a valid session, it will never dangling. | |
| * A SessionHandle always refers to a valid session, it is never dangling. |
| {} | ||
| bool operator==(const VariantSecureSession & that) const { return mLocalSessionId == that.mLocalSessionId; } | ||
|
|
||
| // Include a ref to session manager to make the handler easy to use. |
There was a problem hiding this comment.
Suggested change
| // Include a ref to session manager to make the handler easy to use. | |
| // Include a ref to session manager to make the handle easy to use. |
| bool operator==(const VariantSecureSession & that) const { return mLocalSessionId == that.mLocalSessionId; } | ||
|
|
||
| // Include a ref to session manager to make the handler easy to use. | ||
| // Replace these field with a raw pointer to the actual secure session, once we have figured out how to prevent dangling |
There was a problem hiding this comment.
Suggested change
| // Replace these field with a raw pointer to the actual secure session, once we have figured out how to prevent dangling | |
| // Replace these fields with a raw pointer to the actual secure session, once we have figured out how to prevent dangling |
Comment on lines
+66
to
+67
| SessionManager & mSessionManager; | ||
| uint16_t mLocalSessionId; |
There was a problem hiding this comment.
So after these changes a SessionHandle is still two words, right?
|
@jepenven-silabs we can access these field inside |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This stale pull request has been automatically closed. Thank you for your contributions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
SessionHandleis too big, it contains to name unnecessary fields.Change overview
Variantfor each supported sessionSecureSession, only storeLocalSessionIdin the session handle, because it is unique, and can be used to look up a session.Testing
Verifed by unit-tests.