Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PairingSession::GetSessionType returns incorrect result #13711

Closed
mlepage-google opened this issue Jan 19, 2022 · 3 comments · Fixed by #13772
Closed

PairingSession::GetSessionType returns incorrect result #13711

mlepage-google opened this issue Jan 19, 2022 · 3 comments · Fixed by #13772
Assignees
@turon @emargolis
Labels
V1.0

Comments

@mlepage-google
Copy link
Contributor

Problem

During commissioning, by the time SessionManager::NewPairing (in SessionManager.cpp)
calls PairingSession::GetSessionType to pass it into SecureSessionTable::CreateNewSecureSession,
the value is not set.

You can see this by logging the value and the PairingSession pointer at that location, it's 0 (undefined), not 1 (PASE) or 2 (CASE).

You can add more logging of that value and this pointer in places (e.g. PairingSession::SetSecureSessionType, called by PASESession constructor) to see it was set.

Adding logging of that value and this pointer in PairingSession::Clear reveals it is cleared multiple times between being set and being read, so this is probably why it's not returning the correct result when finally used.

Proposed Solution

Need to determine what the value is used for, whether it matters that it becomes unset, why Clear is called so frequently, etc.
@msandstedt msandstedt mentioned this issue Jan 19, 2022
Merged
@mlepage-google
Copy link
Contributor Author

mlepage-google commented Jan 20, 2022 •
edited
Loading

@emargolis is it as simple as Clear should not reset the session type, since that shouldn't change?

@emargolis emargolis mentioned this issue Jan 20, 2022
Merged
@emargolis
Copy link
Contributor

emargolis commented Jan 20, 2022 •
edited
Loading

@mlepage-google I agree with your assessment that Clear() shouldn't reset the session type.

It is fixed in #13772

@mlepage-google
Copy link
Contributor Author

Thank you!

@woody-apple woody-apple linked a pull request Jan 21, 2022 that will close this issue
Don't Clear mSecureSessionType on PairingSession::Clear() #13772
Merged
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Apr 13, 2022
@tcarmelveilleux
Fix spec-compliance of AddNOC by removing stale code
2d12d15 
- With Issue project-chip#13711, it was assumed only PASE sessions needed to adopt
  FabricIndex of new fabric after AddNOC. This was incorrect, since
  AddNOC is legal over CASE. There was a TODO left.

This PR:

- Removes the TODO and associated incorrect comment
- Renames "NewFabric" to "AdoptFabricIndex" in SecureSession
- Cleans-up some logging in NOC cluster and FabricTable
  that was found while doing this work

Issue project-chip#13711

Testing done:
- All cert tests still pass
- Unit tests pass
- It's not possible to do a full commissioning over CASE yet
  with YAML so I can't make a YAML test case, the code delta
  on logic is such that by inspection it would do the right thing.
@tcarmelveilleux tcarmelveilleux mentioned this issue Apr 13, 2022
Merged
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Apr 13, 2022
@tcarmelveilleux
Fix spec-compliance of AddNOC by removing stale code
6461368 
- With Issue project-chip#13711, it was assumed only PASE sessions needed to adopt
  FabricIndex of new fabric after AddNOC, but the door was left open
  until project-chip#13711 was fixed. It is now fixed so the TODO is gone and
  PASE-only is enforced.

This PR:

- Removes the TODO and associated comment, and enforces PASE
- Renames "NewFabric" to "AdoptFabricIndex" in SecureSession
- Cleans-up some logging in NOC cluster and FabricTable
  that was found while doing this work

Issue project-chip#13711

Testing done:
- All cert tests still pass
- Unit tests pass
- It's not possible to do a full commissioning over CASE yet
  with YAML so I can't make a YAML test case, the code delta
  on logic is such that by inspection it would do the right thing.
tcarmelveilleux added a commit that referenced this issue Apr 14, 2022
@tcarmelveilleux
Fix spec-compliance of AddNOC by removing stale code (#17360)
013421d 
- With Issue #13711, it was assumed only PASE sessions needed to adopt
  FabricIndex of new fabric after AddNOC, but the door was left open
  until #13711 was fixed. It is now fixed so the TODO is gone and
  PASE-only is enforced.

This PR:

- Removes the TODO and associated comment, and enforces PASE
- Renames "NewFabric" to "AdoptFabricIndex" in SecureSession
- Cleans-up some logging in NOC cluster and FabricTable
  that was found while doing this work

Issue #13711

Testing done:
- All cert tests still pass
- Unit tests pass
- It's not possible to do a full commissioning over CASE yet
  with YAML so I can't make a YAML test case, the code delta
  on logic is such that by inspection it would do the right thing.
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@turon turon

@emargolis emargolis

Labels
V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Don't Clear mSecureSessionType on PairingSession::Clear() emargolis/connectedhomeip-1
4 participants
@turon @emargolis @woody-apple @mlepage-google