[PASESession] TLV Reading Fixes #37383
Open
+149
−74
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
|
Review changes with |
Alami-Amine
force-pushed
the
AA/PaseTLVFixes
branch
from
6fd15f1 to
59363de
Compare
|
PR #37383: Size comparison from f957643 to 59363de Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Alami-Amine
changed the title
yunhanw-google
approved these changes
Feb 4, 2025
Alami-Amine
force-pushed
the
AA/PaseTLVFixes
branch
from
59363de to
fa04a11
Compare
|
PR #37383: Size comparison from 577b3f2 to fa04a11 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Feb 6, 2025
| mExchangeCtxt.Value()->GetSessionHandle()->AsUnauthenticatedSession()->SetRemoteSessionParameters( | ||
| GetRemoteSessionParameters()); | ||
|
|
||
| err = tlvReader.Next(); |
There was a problem hiding this comment.
The next requirement decouples the error set where we have:
- set it on line 554 OR
- set it on line 562 if we have a kResponderSessionParams OR
- set it on line 584 OR
- set it on line 592 if we have a kResponderSessionParams
Then check it on line 597
This feels hard to follow. Could we centralize the kResponderSessionParams handling? Could we extract it to a method that reads and rolls back if it cannot find it or have some other organization so that code always reads like err = SomeNext(); VerifyOr... (err == ...) ?
There was a problem hiding this comment.
I extracted it to a method :ReadSessionParamsIfPresent and made sure a VerifyOr is there after each call of it.
andy31415
reviewed
Feb 6, 2025
| // happen in the future if additional elements are added to the specification. | ||
| VerifyOrExit(err == CHIP_END_OF_TLV || err == CHIP_NO_ERROR, ); | ||
|
|
||
| // Exit Container will fail (return CHIP_END_OF_TLV) if the received encoded message is not properly terminated with an |
There was a problem hiding this comment.
What is the purpose of this comment in helping me read the code? It is true that ExitContainer returns a CHIP_ERROR so can fail. Why does this comment explicitly call out the CHIP_END_OF_TLV error and when it occurs?
Same comment for the copies of this below/throughout.
There was a problem hiding this comment.
Just to clarify the importance of ExitContainer()? that having it serves as a check for a missing EndOfContainer terminator in the received message. should i reword it or remove it?
There was a problem hiding this comment.
is this better?
Suggested change
| // Exit Container will fail (return CHIP_END_OF_TLV) if the received encoded message is not properly terminated with an | |
| // ExitContainer() acts as a safeguard to ensure that the received encoded message | |
| // is properly terminated with an EndOfContainer TLV element. If the terminator is missing, CHIP_END_OF_TLV is returned. |
tehampson
reviewed
Feb 6, 2025
Alami-Amine
force-pushed
the
AA/PaseTLVFixes
branch
from
437da5b to
3a79525
Compare
Alami-Amine
force-pushed
the
AA/PaseTLVFixes
branch
from
3a79525 to
843836f
Compare
|
PR #37383: Size comparison from 8ec9b41 to 843836f Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tehampson
approved these changes
Feb 6, 2025
jmartinez-silabs
approved these changes
Feb 10, 2025
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.
Fixes TLV Reading of Containers often missed a call to ExitContainer #36959 : Making sure received encoded messages are properly terminated.
Adding Sanity Checks that the read TLV data has the expected number of bytes:
Adding symbolic TLV Tags
Testing