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.

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

Some cleanup in secure session manager code #4296

Merged
merged 5 commits into from
Jan 11, 2021

Conversation

pan-apple
Copy link
Contributor

@pan-apple pan-apple commented Jan 8, 2021

Problem

SecureSessionMgr::SendMessage() function has grown too big in size, and needs some refactoring.

Summary of Changes

  • Moved code to encrypt message to its own function
  • Moved code to extract packet header from an pre-encrypted message out of the common function
  • Reorganized code to reduce conditional statements and blocks
  • Fixed a memory leak (if mTransportMgr->SendMessage failed, it could have leaked the retained buffer)

src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
@pan-apple pan-apple requested a review from yufengwangca January 8, 2021 16:06
@github-actions
Copy link

github-actions bot commented Jan 8, 2021

Size increase report for "esp32-example-build" from cb23e0c

File Section File VM
chip-all-clusters-app.elf .flash.text 40 40
chip-all-clusters-app.elf .flash.rodata -24 -24
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_loc,0,1653
.debug_info,0,1213
.debug_line,0,759
.debug_str,0,335
.strtab,0,171
.flash.text,40,40
.debug_frame,0,24
[Unmapped],0,24
.symtab,0,16
.debug_abbrev,0,14
.debug_aranges,0,8
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,2
.shstrtab,0,-3
.flash.rodata,-24,-24
.debug_ranges,0,-48


@github-actions
Copy link

github-actions bot commented Jan 8, 2021

Size increase report for "nrfconnect-example-build" from cb23e0c

File Section File VM
chip-lock.elf text 44 44
chip-lock.elf shell_root_cmds_sections -12 -12
chip-lock.elf rodata -16 -20
chip-lighting.elf text 40 40
chip-lighting.elf shell_root_cmds_sections -8 -8
chip-lighting.elf rodata -24 -20
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,1126
.debug_loc,0,861
.debug_line,0,380
.debug_str,0,335
.strtab,0,171
.symtab,0,64
.debug_frame,0,60
text,44,44
.debug_ranges,0,16
.debug_abbrev,0,14
.debug_aranges,0,8
.shstrtab,0,1
shell_root_cmds_sections,-12,-12
rodata,-20,-16

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,1126
.debug_loc,0,865
.debug_line,0,384
.debug_str,0,335
.strtab,0,171
.symtab,0,64
.debug_frame,0,60
text,40,40
.debug_ranges,0,16
.debug_abbrev,0,14
.debug_aranges,0,8
.shstrtab,0,1
shell_root_cmds_sections,-8,-8
rodata,-20,-24

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize


src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Show resolved Hide resolved
@@ -92,136 +92,157 @@ CHIP_ERROR SecureSessionMgr::SendMessage(NodeId peerNodeId, System::PacketBuffer
CHIP_ERROR SecureSessionMgr::SendMessage(PayloadHeader & payloadHeader, NodeId peerNodeId, System::PacketBufferHandle msgBuf,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call these ResendMessage?

I think there should be a difference between stuff taking headers and things that decode packet/payload headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about SendEncryptedMessage? From the API perspective, it does not know the CRMP is using it for resends.

src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
src/transport/SecureSessionMgr.cpp Outdated Show resolved Hide resolved
@pan-apple pan-apple requested a review from yufengwangca January 8, 2021 18:44
@pan-apple
Copy link
Contributor Author

@saurabhst, @jelderton, @BroderickCarlin, do you have any comments?

@rwalker-apple rwalker-apple merged commit 832a4e1 into project-chip:master Jan 11, 2021
@pan-apple pan-apple deleted the secure-sess-mgr-cleanup branch January 11, 2021 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants