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

Remove CHIP buffer size check within shell and leave this it to lower layer #7523

Merged
merged 2 commits into from
Jun 14, 2021
Merged

Remove CHIP buffer size check within shell and leave this it to lower layer #7523

merged 2 commits into from
Jun 14, 2021

Conversation

yufengwangca
Copy link
Contributor

Problem

What is being fixed?

We should not take Payload size check within the shell application since it does not take default header size into account,

For example, I set the buffer size to 1279, which passed the payload size check within Shell, but still failed with CHIP Error 4004

> send -s 1279 192.168.86.171
IP address: 192.168.86.171
CHIP:IN: TransportMgr initialized
CHIP:IN: TransportMgr initialized
CHIP:DL: writing settings to file (/tmp/chip_counters.ini.tmp)
CHIP:DL: failed to rename (/tmp/chip_counters.ini.tmp), Operation not permitted (1)
CHIP:IN: local node id is 0x000000000001b669
CHIP:IN: Connection from 'UDP:192.168.86.171:11097' expired
CHIP:IN: New pairing for device 0x0000000000bc5c01, key 0!!
Establish secure session succeeded

Send CHIP message with payload size: 1279 bytes to Node: 12344321
CHIP:EM: Failed to send message with err CHIP Error 4004 (0x00000FA4): Message too long
Send CHIP message failed, err: CHIP Error 4004 (0x00000FA4): Message too long
Send failed with error: CHIP Error 4004 (0x00000FA4): Message too long
Done
>

Change overview

  1. Remove CHIP buffer size check within shell and leave this it to lower layer
  2. Use stack buffer instead of dynamic memory to accommodate fixed length message.

Testing

How was this tested? (at least one bullet point required)
Set the Payload size to 1281 which exceed the previous kMaxPayloadSize

> ping -s 1281 192.168.86.171
IP address: 192.168.86.171
[1623341443.860683][150131] CHIP:IN: TransportMgr initialized
[1623341443.860804][150131] CHIP:IN: TransportMgr initialized
[1623341443.861217][150131] CHIP:DL: writing settings to file (/tmp/chip_counters.ini.tmp)
[1623341443.861499][150131] CHIP:DL: failed to rename (/tmp/chip_counters.ini.tmp), Operation not permitted (1)
[1623341443.861537][150131] CHIP:IN: local node id is 0x000000000001B669
[1623341443.861637][150131] CHIP:IN: Connection from 'UDP:192.168.86.171:11097' expired
[1623341443.861675][150131] CHIP:IN: New pairing for device 0x0000000000bc5c01, key 0!!
Establish secure session succeeded

Send echo request message with payload size: 1281 bytes to Node: 12344321
[1623341443.861834][150131] CHIP:EM: Failed to send message with err CHIP Error 4004 (0x00000FA4): Message too long
Send echo request failed, err: CHIP Error 4004 (0x00000FA4): Message too long
Send request failed: CHIP Error 4004 (0x00000FA4): Message too long
Ping failed with error: CHIP Error 4004 (0x00000FA4): Message too long
Done
>

Confirm the command send failed at lower layer due to "Message too long"

examples/shell/shell_common/cmd_ping.cpp Outdated Show resolved Hide resolved
examples/shell/shell_common/cmd_ping.cpp Outdated Show resolved Hide resolved
examples/shell/shell_common/cmd_ping.cpp Outdated Show resolved Hide resolved
examples/shell/shell_common/cmd_send.cpp Outdated Show resolved Hide resolved
@boring-cyborg boring-cyborg bot added the system label Jun 11, 2021
@pullapprove pullapprove bot requested a review from sagar-apple June 11, 2021 16:59
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

Much better, thank you! There's one remaining issue in SystemPacketBuffer and it's a little weird to be sending random

src/system/SystemPacketBuffer.cpp Outdated Show resolved Hide resolved
VerifyOrExit(!payloadBuf.IsNull(), err = CHIP_ERROR_NO_MEMORY);

payloadBuf->SetDataLength(gPingArguments.GetEchoReqSize());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please memset the part of the buffer up to the new data length to 0 or otherwise initialize it. Otherwise we are possibly leaking data that was in previous messages. Might be OK here, but in general it's a really bad pattern and we should not have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acked

VerifyOrExit(!payloadBuf.IsNull(), err = CHIP_ERROR_NO_MEMORY);

payloadBuf->SetDataLength(gSendArguments.GetPayloadSize());
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, we should not be sending uninitialized data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Acked

@github-actions
Copy link

Size increase report for "nrfconnect-example-build" from 6f297c5

File Section File VM
chip-shell.elf device_handles 8 8
chip-shell.elf rodata -16 -20
chip-shell.elf text -56 -56
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_line,0,83
.debug_info,0,65
.debug_ranges,0,48
device_handles,8,8
rodata,-20,-16
text,-56,-56
.debug_str,0,-107
.debug_loc,0,-361

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

sections,vmsize,filesize


Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

Much better, thank you!

@bzbarsky-apple bzbarsky-apple merged commit 7124079 into project-chip:master Jun 14, 2021
@yufengwangca yufengwangca deleted the pr/shell/ping branch June 14, 2021 16:39
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
… layer (project-chip#7523)

* Remove CHIP buffer size check within shell and leave this check to transport layer

* Address review comments
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.

4 participants