Make DecodeMRPParametersIfPresent() and ParseSigma1() methods static + fix for TLV reading corner case
#36956
+321
−49
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/DecodeParams
branch
from
199286a to
5023fe6
Compare
Alami-Amine
force-pushed
the
AA/DecodeParams
branch
from
5023fe6 to
1e3e1a2
Compare
|
PR #36956: Size comparison from 1b4c56c to 1e3e1a2 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
Alami-Amine
changed the title
DecodeMRPParametersIfPresent() and ParseSigma1() methods static
bzbarsky-apple
approved these changes
Jan 3, 2025
| outParsedSigma1.initiatorMrpParamsPresent = true; | ||
| ReturnErrorOnFailure(DecodeSessionParametersIfPresent(AsTlvContextTag(Sigma1Tags::kInitiatorSessionParams), tlvReader, | ||
| outParsedSigma1.initiatorSessionParams)); | ||
| outParsedSigma1.initiatorSessionParamsPresent = true; |
There was a problem hiding this comment.
Should this also set outParsedSigma1.initiatorSessionParamsPresent to false if the params are not in fact present? Is it allowed to reuse a ParsedSigma1 for multiple calls to this function? Seems like the answers should be "yes" and "maybe" respectively....
There was a problem hiding this comment.
I believe that a certain instance of ParsedSigma1 will never be reused for another call to ParseSigma1(), since ParsedSigma1 is declared on the stack inside HandleSigma1() which only calls ParseSigma1() once, if we were to call HandleSigma1() again, an instance of ParsedSigma1 will have gone out of scope, or am I missing something?
There was a problem hiding this comment.
Should this also set
outParsedSigma1.initiatorSessionParamsPresentto false if the params are not in fact present?
I am guessing we are setting this to true because if (err == CHIP_NO_ERROR && tlvReader.GetTag() == AsTlvContextTag(Sigma1Tags::kInitiatorSessionParams)). I guess the corner case is that the initiator set this to an empty struct. In this corner case is the logic of setting outParsedSigma1.initiatorSessionParamsPresent to true correct? If so maybe renaming initiatorSessionParamsPresent to initiatorSessionParamStructPresent or something to that affect.
Is it allowed to reuse a
ParsedSigma1for multiple calls to this function?
It looks like ParsedSigma1 is protected (private, but also accessible by test so protected)
Edit: For the first question there is also the corner case is that initiator set Sigma1Tags::kInitiatorSessionParams, but within that structure it has a field that something else doesn't understand (ie, new matter device communicating with older matter device that doesn't know about future parameters in the Session Params struct)
There was a problem hiding this comment.
since ParsedSigma1 is declared on the stack inside HandleSigma1
But the whole point of a separate method like this is that it might be called from places that are not HandleSigma1, no? But if the intent is that callers must always use a fresh ParsedSigma1 when calling this function, that seems fine but should be documented.
There was a problem hiding this comment.
@bzbarsky-apple well the point of this method was mostly to improve testability. So all other callers will be Test Code, I assume we shouldn't make such a decision just for Test Code.
I added a note in the .h
There was a problem hiding this comment.
-
a related question to your 1st Corner case: is it an issue if we set mRemoteSessionParams to empty parameters though? if it isn't, I agree to change to
initiatorSessionParamStructPresentsince its clearer -
corner case 2: I believe this is already handled within
DecodeSessionParametersIfPresent()and improved with this PR: since if the old matter device receives an additional TLV element within the SessionParameters that it doesnt recognize: we would get CHIP_NO_ERROR at exit, and then we would callExitContainer, and withinParseSigma1()that same sequence will happen. is that what you meant?
There was a problem hiding this comment.
Essentially, today with your PR, both corner cases I mention do the same thing were it uses the default SessionParameter values. I don't have any strong opinions on this and am okay with how things are done in your PR. But we can technically detect the first one and return an error if we would like. This can also be done in a follow up PR is we are to head in that direction.
@bzbarsky-apple do you have an opinion on what should happen if the initiator has an empty struct (corner case 1?
Essentially my comment on the two corner cases was more a discussion of what should initiatorSessionParamPresent, or whatever we end up calling it, be set to and what are we trying to do with it. If in both the corner cases I am talking initiatorSessionParamPresent is set to true, then what you have done in the PR is correct. I only brought up the corner case because Boris was asking about when it should be set to false
…nParameters struct has extra TLV elements that were added to the spec, Yet the TLV struct is not terminated with an EndOfContainer Element. In this Case, we should make sure that ExitContainer so that an Error is triggered.
Alami-Amine
force-pushed
the
AA/DecodeParams
branch
from
c9dc062 to
144b4c1
Compare
Alami-Amine
changed the title
DecodeMRPParametersIfPresent() and ParseSigma1() methods static DecodeMRPParametersIfPresent() and ParseSigma1() methods static + fix for corner case
|
PR #36956: Size comparison from bba390a to 144b4c1 Full report (11 builds for cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
Alami-Amine
force-pushed
the
AA/DecodeParams
branch
from
144b4c1 to
d77c3ea
Compare
|
PR #36956: Size comparison from bba390a to d77c3ea Full report (14 builds for cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, tizen)
|
Alami-Amine
force-pushed
the
AA/DecodeParams
branch
from
d77c3ea to
4cdfd0f
Compare
|
PR #36956: Size comparison from bba390a to 4cdfd0f Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
tehampson
reviewed
Jan 6, 2025
tehampson
approved these changes
Jan 6, 2025
|
PR #36956: Size comparison from bba390a to c6018c1 Full report (47 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, linux, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
Alami-Amine
changed the title
DecodeMRPParametersIfPresent() and ParseSigma1() methods static + fix for corner caseDecodeMRPParametersIfPresent() and ParseSigma1() methods static + fix for TLV reading corner case
|
PR #36956: Size comparison from bba390a to a50db92 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
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.
Changes in the PR:
decoupling
DecodeMRPParametersIfPresent()from class state and making it static; this is needed to make the factored out TLV Parsing Functions in CASESession and PASESession self-contained and indepedent of class state.Fix for Corner Case in
DecodeMRPParametersIfPresent: If the received TLV-encoded SessionParameters structure contains extra TLV elements not known by the receiver (newly added to the specification) but is not terminated with an EndOfContainer element, we must ENSURE thatExitContaineris called to ensure that an error is triggered.Added Unit Tests, which also covers above Corner case.
Change the name of
DecodeMRPParametersIfPresent()toDecodeSessionParametersIfPresent()since it is not ONLY decoding MRP but also other Session Parameters (I believe in Matter 1.2 Session Parameters only Included MRP hence the name?)Make
ParseSigma1()static and completely self-contained.Testing
Originally posted by @bzbarsky-apple in #36679 (comment)