Fix CRMP resend null out retained buffer #7312

kghost · 2021-06-02T12:28:27Z

Problem

There is a bug that resend message will null out CRMP retained buffer. Due to this line:

dispatcher->ResendMessage(rc->GetExchangeContext()->GetSecureSession(), std::move(entry->retainedBuf), &entry->retainedBuf)

The code is a little strange, hard to understand, and error-prone.

Change overview

The PR split send message into 2 steps:

PrepareMessage: create a message buffer which can be send multiple times without changing.
SendPreparedMessage: send the prepared message, this function is re-entrance.

Then SendMessage becomes:

EncryptedPacketBufferHandle preparedMessage;
ReturnErrorOnFailure(PrepareMessage(session, payloadHeader, std::move(message), preparedMessage));
ReturnErrorOnFailure(SendPreparedMessage(session, preparedMessage));

ResendMessage becomes:

ReturnErrorOnFailure(SendPreparedMessage(session, preparedMessage));

This makes all things clear and easy to understand.

Testing

Manually tested with echo and im

andy31415 · 2021-06-02T14:07:48Z

src/transport/SecureSessionMgr.h

+     *    3. Encode the packet header and prepend it to message.
+     *   Returns a prepared message in preparedMessage.
+     */
+    CHIP_ERROR PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader, System::PacketBufferHandle && msgBuf,


The way I understand it, the method takes a buffer of data and encrypts its + adds the packet header. I find "prepare" a bit vague and it forces me to read through the comments to understand what the method does.

Is it correct that this is the one way we expect to convert a packet handle to an encrypted one? How about in this case:

EncryptPayload or BuildEncryptedMessagePayload

and

SendEncryptedPayload ?

This way I do not read comments, I understand that giving a buffer, I encrypt it and send it.

This API is also handling unsecure message in SessionEstablishmentExchangeDispatch.cpp, So it is not proper to name it BuildEncryptedMessagePayload

And BuildMessagePayload is also a bit strange.

The prepared steps in the comment say "it encrypts the msgBuf".
I am trying to find out if we can get a more descriptive name than "Prepare". From what I see this is that this has:

Input: session, header, messagebuffer

Output: EncryptedMessageBuffer

Which tells me it encrypts and adds a header. Realistically also wondering why we take a sessionhandle instead of just keys if we do encryption, but that is not for this PR.

Could you either update the comment about encryption or find some descriptive name (or both)?

I think missing "override" made me not realize this particular implementation does "encrypt" but others may not. Please add override keyword here and below as needed and then I think the name is ok.

Actually, this particular impl always encrypts. The message dispatch bits can keep Prepare(), etc, but we could name this API something different and clearer.

andy31415

The summary states that manual testing was done and I was about to comment about 'can we also have automated tests please?' However I also see test cases modified, so the code is exercised in unit tests as well.

Could we update the unit tests to be able to detect the bug to make sure it never happens again?

kghost · 2021-06-02T14:30:38Z

Could we update the unit tests to be able to detect the bug to make sure it never happens again?

The logic causing the problem no longer exists, SendPreparedMessage now takes a const PacketHandle, it is unable to modify it. So it is not possible to write a unit test to detect it.

github-actions · 2021-06-02T14:39:35Z

Size increase report for "gn_qpg6100-example-build" from f6af7b9

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	-176	-176

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
[Unmapped],0,176
.debug_aranges,0,-24
.symtab,0,-80
.debug_frame,0,-92
.debug_abbrev,0,-113
.text,-176,-176
.strtab,0,-188
.debug_str,0,-289
.debug_ranges,0,-368
.debug_line,0,-426
.debug_loc,0,-876
.debug_info,0,-1512

andy31415 · 2021-06-02T21:40:11Z

src/transport/SecureSessionMgr.cpp

+    if (state == nullptr)
+    {
+        return CHIP_ERROR_NOT_CONNECTED;
+    };


nit: ";" not needed here

src/messaging/ExchangeMessageDispatch.cpp

src/messaging/ExchangeMessageDispatch.h

bzbarsky-apple · 2021-06-03T21:18:02Z

src/transport/SecureSessionMgr.h

@@ -68,6 +68,8 @@ class EncryptedPacketBufferHandle final : private System::PacketBufferHandle

    uint32_t GetMsgId() const;

+    PacketBufferHandle Retain() const { return PacketBufferHandle::Retain(); }


This is violating the main invariant of this class, which is that you have to call CastToWritable to be able to modify it.

There are two options here:

We make CastToWritable callable on an lvalue and have it do a Retain() internally: PacketBufferHandle CastToWritable() { return PacketBufferHandle::Retain(); }

We add a Retain that returns EncryptedPacketBufferHandle: EncryptedPacketBufferHandle Retain() { return EncryptedPacketBufferHandle(PacketBufferHandle::Retain()); } and then we CastToWritable that thing that Retain returns.

src/transport/SecureSessionMgr.h

bzbarsky-apple · 2021-06-03T21:19:27Z

src/transport/SecureSessionMgr.h

+     *    3. Encode the packet header and prepend it to message.
+     *   Returns a prepared message in preparedMessage.
+     */
+    CHIP_ERROR PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader, System::PacketBufferHandle && msgBuf,


Actually, this particular impl always encrypts. The message dispatch bits can keep Prepare(), etc, but we could name this API something different and clearer.

github-actions · 2021-06-04T14:56:36Z

Size increase report for "nrfconnect-example-build" from 7d54157

File	Section	File	VM
chip-lock.elf	device_handles	-4	-4
chip-lock.elf	rodata	-24	-24
chip-lock.elf	text	-204	-204
chip-shell.elf	text	-64	-64

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_str,0,8555
.debug_info,0,3431
.debug_loc,0,57
.shstrtab,0,2
device_handles,-4,-4
.debug_aranges,0,-24
rodata,-24,-24
.debug_frame,0,-92
.symtab,0,-96
.strtab,0,-174
text,-204,-204
.debug_line,0,-219
.debug_ranges,0,-368
.debug_abbrev,0,-820

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

sections,vmsize,filesize
.debug_str,0,8795
.debug_info,0,5588
.debug_loc,0,953
.debug_abbrev,0,249
.debug_line,0,143
.shstrtab,0,1
.debug_aranges,0,-16
.symtab,0,-32
.debug_frame,0,-64
text,-64,-64
.debug_ranges,0,-144
.strtab,0,-177

github-actions · 2021-06-04T14:57:27Z

Size increase report for "esp32-example-build" from 7d54157

File	Section	File	VM
chip-all-clusters-app.elf	.flash.rodata	-24	-24
chip-all-clusters-app.elf	.flash.text	-232	-232

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_str,0,8301
.debug_info,0,3001
.xt.prop._ZNK4chip36SessionEstablishmentExchangeDispatch29IsReliableTransmissionAllowedEv,0,76
.xt.prop._ZNK4chip9Messaging23ExchangeMessageDispatch29IsReliableTransmissionAllowedEv,0,76
[Unmapped],0,24
.shstrtab,0,2
.debug_aranges,0,-24
.flash.rodata,-24,-24
.symtab,0,-32
.debug_frame,0,-72
.xt.prop._ZN4chip36SessionEstablishmentExchangeDispatch29IsReliableTransmissionAllowedEv,0,-76
.xt.prop._ZN4chip9Messaging23ExchangeMessageDispatch29IsReliableTransmissionAllowedEv,0,-76
.strtab,0,-174
.debug_ranges,0,-176
.flash.text,-232,-232
.debug_loc,0,-467
.debug_line,0,-477
.debug_abbrev,0,-1158

woody-apple · 2021-06-04T23:55:53Z

/rebase

kghost · 2021-06-10T14:11:02Z

@bzbarsky-apple I have upated a new patch to resolve your suggestion, please take a look

bzbarsky-apple

My apologies for the lag; I missed the comments being addressed. Thank you for the direct ping!

src/transport/SecureSessionMgr.h

Co-authored-by: Boris Zbarsky <[email protected]>

woody-apple · 2021-06-16T04:07:31Z

/rebase

woody-apple · 2021-06-16T15:32:49Z

@saurabhst @msandstedt @andy31415 ?

* Fix CRMP resend null out retained buffer * Resolve comments * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>

boring-cyborg bot added the transport label Jun 2, 2021

kghost requested a review from bzbarsky-apple June 2, 2021 12:29

kghost force-pushed the fix-rref branch 2 times, most recently from 59af2c3 to c052e28 Compare June 2, 2021 12:56

restyled-io bot mentioned this pull request Jun 2, 2021

Restyle Fix CRMP resend null out retained buffer #7314

Closed

kghost force-pushed the fix-rref branch from c052e28 to 494bdb5 Compare June 2, 2021 13:02

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang and saurabhst June 2, 2021 13:07

pullapprove bot added the review - pending label Jun 2, 2021

andy31415 reviewed Jun 2, 2021

View reviewed changes

bzbarsky-apple requested changes Jun 3, 2021

View reviewed changes

andy31415 added the Tested manually label Jun 3, 2021

kghost force-pushed the fix-rref branch from 494bdb5 to 07f4b23 Compare June 4, 2021 12:02

woody-apple force-pushed the fix-rref branch from 07f4b23 to e0b1552 Compare June 5, 2021 00:15

bzbarsky-apple approved these changes Jun 10, 2021

View reviewed changes

src/transport/SecureSessionMgr.h Outdated Show resolved Hide resolved

src/transport/SecureSessionMgr.h Outdated Show resolved Hide resolved

andy31415 approved these changes Jun 14, 2021

View reviewed changes

kghost added 2 commits June 15, 2021 17:28

Fix CRMP resend null out retained buffer

22a77da

Resolve comments

4c3e0d0

Apply suggestions from code review

5f97d0f

Co-authored-by: Boris Zbarsky <[email protected]>

kghost force-pushed the fix-rref branch from 73b1580 to 5f97d0f Compare June 15, 2021 11:57

msandstedt approved these changes Jun 16, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Jun 16, 2021

woody-apple merged commit 23d3a6d into project-chip:master Jun 16, 2021

kghost deleted the fix-rref branch June 16, 2021 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CRMP resend null out retained buffer #7312

Fix CRMP resend null out retained buffer #7312

kghost commented Jun 2, 2021

andy31415 Jun 2, 2021

kghost Jun 2, 2021 •

edited

Loading

andy31415 Jun 2, 2021

andy31415 Jun 2, 2021

bzbarsky-apple Jun 3, 2021

andy31415 left a comment

kghost commented Jun 2, 2021 •

edited

Loading

github-actions bot commented Jun 2, 2021

andy31415 Jun 2, 2021

bzbarsky-apple Jun 3, 2021

bzbarsky-apple Jun 3, 2021

github-actions bot commented Jun 4, 2021

github-actions bot commented Jun 4, 2021

woody-apple commented Jun 4, 2021

kghost commented Jun 10, 2021

bzbarsky-apple left a comment

woody-apple commented Jun 16, 2021

woody-apple commented Jun 16, 2021

		@@ -68,6 +68,8 @@ class EncryptedPacketBufferHandle final : private System::PacketBufferHandle

		uint32_t GetMsgId() const;

		PacketBufferHandle Retain() const { return PacketBufferHandle::Retain(); }

Fix CRMP resend null out retained buffer #7312

Fix CRMP resend null out retained buffer #7312

Conversation

kghost commented Jun 2, 2021

Problem

Change overview

Testing

andy31415 Jun 2, 2021

Choose a reason for hiding this comment

kghost Jun 2, 2021 • edited Loading

Choose a reason for hiding this comment

andy31415 Jun 2, 2021

Choose a reason for hiding this comment

andy31415 Jun 2, 2021

Choose a reason for hiding this comment

bzbarsky-apple Jun 3, 2021

Choose a reason for hiding this comment

andy31415 left a comment

Choose a reason for hiding this comment

kghost commented Jun 2, 2021 • edited Loading

github-actions bot commented Jun 2, 2021

andy31415 Jun 2, 2021

Choose a reason for hiding this comment

bzbarsky-apple Jun 3, 2021

Choose a reason for hiding this comment

bzbarsky-apple Jun 3, 2021

Choose a reason for hiding this comment

github-actions bot commented Jun 4, 2021

github-actions bot commented Jun 4, 2021

woody-apple commented Jun 4, 2021

kghost commented Jun 10, 2021

bzbarsky-apple left a comment

Choose a reason for hiding this comment

woody-apple commented Jun 16, 2021

woody-apple commented Jun 16, 2021

kghost Jun 2, 2021 •

edited

Loading

kghost commented Jun 2, 2021 •

edited

Loading