Careful resume structs + encode/decode code #5131
Conversation
…default - Uplevel server can accept v1 tickets but always issue v2 tickets - V2 resumption ticket has a 64-byte buffer for usage in the near future - Updated ticket tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5131 +/- ##
==========================================
- Coverage 87.41% 86.23% -1.18%
==========================================
Files 59 59
Lines 18086 18233 +147
==========================================
- Hits 15809 15723 -86
- Misses 2277 2510 +233 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- CR Struct encoded/decoded to/from variable length buffers - CR encoding and decoding added to custom server resumption ticket logic - Added test cases for the code changes
sm-msft
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Add info about updating sidecar to build documentation - Address MacOS build failures
…ures - Additional tests to cover encoding/decoding careful resume struct
|
I have added lots of unit tests to provide coverage for the new code. The new code is not yet hooked up to the rest of the mainline code which typically gets exercised in FVTs etc. I am unsure if the unit tests are running as part of the automation and whether this tool captures that coverage data.
|
- Additional failure unit tests
| @@ -450,11 +450,22 @@ CXPLAT_STATIC_ASSERT( | |||
| // | |||
| #define QUIC_DEFAULT_SERVER_RESUMPTION_LEVEL QUIC_SERVER_NO_RESUME | |||
|
|
|||
|
|
|||
| BOOLEAN | ||
| QuicCryptoDecodeCRState( | ||
| _Out_ QUIC_CONN_CAREFUL_RESUME_STATE * CarefulResumeState, | ||
| _In_reads_(CRBufLength) const uint8_t * Buffer, |
There was a problem hiding this comment.
nit: style. We tie the * to the type.
There was a problem hiding this comment.
(one more argument for having a .clang-format :) )
| TicketCursor = QuicVarIntEncode(AppDataLength, TicketCursor); | ||
| CxPlatCopyMemory(TicketCursor, NegotiatedAlpn, AlpnLength); | ||
| TicketCursor += AlpnLength; | ||
| CxPlatCopyMemory(TicketCursor, EncodedHSTP + CxPlatTlsTPHeaderSize, EncodedTPLength); | ||
| TicketCursor += EncodedTPLength; | ||
|
|
||
| if (NULL != CarefulResumeState) { | ||
| QuicCryptoEncodeCRState(EncodedCRLength, CarefulResumeState, Connection, &EncodedCRLength, TicketCursor); |
There was a problem hiding this comment.
Assuming the comment above has been fixed, on failure, EncodedCRLength == 0.
So *TicketLength = TotalTicketLength won't be correct anymore.
There was a problem hiding this comment.
This function is specifically written so that it is supplied a certain min amount of buffer to be filled.
We hit asserts in the failure code paths. We are not expected to ship with that error. There is no recovery path here either. I dont want include extra validation that is not needed here.
There was a problem hiding this comment.
I don't understand why we have an out parameter in this function for the size if the function assumes it receive the correct amount of memory. Either the function assumes the caller provide a valid buffer and doesn't need to check it, or it doesn't, and it must handle failure.
|
Discussed further comments over teams. Will be merging this change. |
sm-msft
dismissed
guhetier’s stale review
Discussed this over Teams.
Guillaume's comment:
"The value of annotations on the parameters is mostly about letting the analysis in the caller happen properly.
In the function itself, any assert or assume is good with me."
Addresses these sub-feature requests
Description
-- Uplevel server can accept v1 tickets but always issue v2 tickets
-- Updated ticket tests
Testing
Ran updated unit tests + running pipeline tests
Documentation
N/A