[fix] emberAfCopyString should accept size_t size[TE4] #8241

erjiaqing · 2021-07-09T05:54:09Z

Problem

emberAfCopyString accepts uint8_t currently, however we should be able to pass longer buffer to emberAfCopyString.

Change overview

- void emberAfCopyString(uint8_t * dest, const uint8_t * src, uint8_t size);
+ void emberAfCopyString(uint8_t * dest, const uint8_t * src, size_t size);

Testing

Manaully tested.

This actually fixed the issue that lighting app will return empty string for some fields.

Run lighting app with this PR:

chip-device-ctrl > zclread Basic VendorName 1 0 0
[1625810123.223412][2381993:2381993] CHIP:IN: Marking old secure session for device 0x0000000000000001 as expired
[1625810123.223448][2381993:2381993] CHIP:CTL: OnConnectionExpired was called for unknown device, ignoring it.
[1625810123.223466][2381993:2381993] CHIP:IN: New secure session created for device 0x0000000000000001, key 0!!
[1625810123.223547][2381993:2381993] CHIP:DMG: SendReadRequest: Client[0] [ INIT]
[1625810123.223640][2381993:2381993] CHIP:IN: Secure message was encrypted: Msg ID 6
[1625810123.223652][2381993:2381993] CHIP:IN: Encrypted message 0x1094fb8 from 0x000000000001B669 to 0x0000000000000001 of type 2 and protocolId 5 on exchange 39021.
[1625810123.223665][2381993:2381993] CHIP:IN: Sending msg 0x1094fb8 to 0x0000000000000001 at utc time: 360074982 msec
[1625810123.223675][2381993:2381993] CHIP:IN: Sending secure msg on generic transport
[1625810123.223760][2381993:2381993] CHIP:IN: Secure msg send status No Error
[1625810123.223787][2381993:2381993] CHIP:DMG: Client[0] moving to [AwaitingResponse]
[1625810123.225107][2381993:2382000] CHIP:IN: Secure transport received message destined to fabric 0, node 0x000000000001B669. Key ID 1
[1625810123.225175][2381993:2382000] CHIP:EM: Received message of type 5 and protocolId 5 on exchange 39021
[1625810123.225212][2381993:2382000] CHIP:EM: Rxd Ack; Removing MsgId:00000006 from Retrans Table
[1625810123.225230][2381993:2382000] CHIP:EM: Removed CHIP MsgId:00000006 from RetransTable
[1625810123.225272][2381993:2382000] CHIP:DMG: ReportData =
[1625810123.225291][2381993:2382000] CHIP:DMG: {
[1625810123.225308][2381993:2382000] CHIP:DMG:  AttributeDataList =
[1625810123.225327][2381993:2382000] CHIP:DMG:  [
[1625810123.225344][2381993:2382000] CHIP:DMG:          AttributeDataElement =
[1625810123.225364][2381993:2382000] CHIP:DMG:          {
[1625810123.225383][2381993:2382000] CHIP:DMG:                  AttributePath =
[1625810123.225408][2381993:2382000] CHIP:DMG:                  {
[1625810123.225432][2381993:2382000] CHIP:DMG:                          NodeId = 0x1,
[1625810123.225451][2381993:2382000] CHIP:DMG:                          EndpointId = 0x0,
[1625810123.225490][2381993:2382000] CHIP:DMG:                          ClusterId = 0x28,
[1625810123.225509][2381993:2382000] CHIP:DMG:                          FieldTag = 0x1,
[1625810123.225526][2381993:2382000] CHIP:DMG:                  }
[1625810123.225548][2381993:2382000] CHIP:DMG: 
[1625810123.225570][2381993:2382000] CHIP:DMG:                  Data = "TEST_VENDOR", 
[1625810123.225590][2381993:2382000] CHIP:DMG:                  DataElementVersion = 0x0,
[1625810123.225609][2381993:2382000] CHIP:DMG:          },
[1625810123.225632][2381993:2382000] CHIP:DMG: 
[1625810123.225650][2381993:2382000] CHIP:DMG:  ],
[1625810123.225672][2381993:2382000] CHIP:DMG: 
[1625810123.225688][2381993:2382000] CHIP:DMG: }
[1625810123.225735][2381993:2382000] CHIP:ZCL: ReadAttributesResponse:
[1625810123.225751][2381993:2382000] CHIP:ZCL:   ClusterId: 0x0028
[1625810123.225770][2381993:2382000] CHIP:ZCL:   attributeId: 0x0001
[1625810123.225786][2381993:2382000] CHIP:ZCL:   status: Success                (0x0000)
[1625810123.225803][2381993:2382000] CHIP:ZCL:   attribute TLV Type: 0x0c
[1625810123.225822][2381993:2382000] CHIP:ZCL:   value: TEST_VENDOR
[1625810123.225850][2381993:2382000] CHIP:ZCL:   attributeValue: (span of length 11)  84 69 83 84 95 86 69 78 68 79 82
[1625810123.225953][2381993:2382000] CHIP:EM: Sending Standalone Ack for MsgId:00000004
[1625810123.226001][2381993:2382000] CHIP:IN: Secure message was encrypted: Msg ID 7
[1625810123.226023][2381993:2382000] CHIP:IN: Encrypted message 0x7fad5affc248 from 0x000000000001B669 to 0x0000000000000001 of type 16 and protocolId 0 on exchange 39021.
[1625810123.226050][2381993:2382000] CHIP:IN: Sending msg 0x7fad5affc248 to 0x0000000000000001 at utc time: 360074985 msec
[1625810123.226071][2381993:2382000] CHIP:IN: Sending secure msg on generic transport
chip-device-ctrl > [1625810123.226154][2381993:2382000] CHIP:IN: Secure msg send status No Error
[1625810123.226176][2381993:2382000] CHIP:EM: Flushed pending ack for MsgId:00000004
[1625810123.226195][2381993:2382000] CHIP:DMG: Client[0] moving to [INIT]
[1625810123.226213][2381993:2382000] CHIP:DMG: Client[0] moving to [UNINIT]

bzbarsky-apple

This actually fixed the issue that lighting app will return empty string for some fields.

Because the buffersize we pass in to typeSensitiveMemCopy overflows uint8_t? But shouldn't the real fix then be in the casting in that function? Because passing a size larger than uint8_t to emberAfCopyString doesn't necessarily make sense as things stand, because the callee will never copy more than uint8_ts-count of bytes.... Unless we just want to make that param size_t or something like that in general?

In any case, if this affects app behavior in an observable way, it sounds like we should be able to add CI for it. Can the problem be reproduced with all-clusters-app, or just with lighing-app?

andy31415 · 2021-07-09T19:32:22Z

I have previously tried to add unit tests for ember and failed - at that time recursive includes (ember lib including gen files) prevented me from creating a library to build tests upon.

msandstedt · 2021-07-09T20:18:19Z

This actually fixed the issue that lighting app will return empty string for some fields.

Because the buffersize we pass in to typeSensitiveMemCopy overflows uint8_t? But shouldn't the real fix then be in the casting in that function? Because passing a size larger than uint8_t to emberAfCopyString doesn't necessarily make sense as things stand, because the callee will never copy more than uint8_ts-count of bytes.... Unless we just want to make that param size_t or something like that in general?

In any case, if this affects app behavior in an observable way, it sounds like we should be able to add CI for it. Can the problem be reproduced with all-clusters-app, or just with lighing-app?

Agreed. The current behavior is:

destination buffer can be any size (large or small)
copied strings will be (silently) truncated if destination size is too small

If we're going to retain that behavior, size_t makes more sense to me. Isn't the bigger problem the silent truncation though? Is that actually what we want?

bzbarsky-apple · 2021-07-09T21:04:51Z

I have previously tried to add unit tests for ember and failed

This is why it matters that this is observable in an end-to-end test with lighting-app. We have end-to-end tests that exercise the sort of thing exercised in #8241 (comment) running in CI, so should be able to test this, I would think.

bzbarsky-apple · 2021-07-09T21:06:12Z

Isn't the bigger problem the silent truncation though? Is that actually what we want?

@vivien-apple has been looking into that a bit recently, I think...

franck-apple · 2021-07-09T21:07:40Z

removing the "TE4 cherrypick" label, while waiting for CSG's validation that it's needed.
Cherrypicking time is nearly over anyway.

erjiaqing · 2021-07-12T02:01:49Z

This actually fixed the issue that lighting app will return empty string for some fields.

Because the buffersize we pass in to typeSensitiveMemCopy overflows uint8_t? But shouldn't the real fix then be in the casting in that function? Because passing a size larger than uint8_t to emberAfCopyString doesn't necessarily make sense as things stand, because the callee will never copy more than uint8_ts-count of bytes.... Unless we just want to make that param size_t or something like that in general?

In any case, if this affects app behavior in an observable way, it sounds like we should be able to add CI for it. Can the problem be reproduced with all-clusters-app, or just with lighing-app?

Yes, we should add more tests between different apps -- I would suggest a follow up.

Changed uint16_t -> size_t for emberAfCopyString

github-actions · 2021-07-12T02:37:38Z

Size increase report for "nrfconnect-example-build" from 0e19e0d

File	Section	File	VM
chip-lock.elf	device_handles	4	4
chip-lock.elf	text	-4	-4

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

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

sections,vmsize,filesize
.debug_line,0,20
.debug_frame,0,8
device_handles,4,4
text,-4,-4
.debug_loc,0,-12

github-actions · 2021-07-12T02:37:56Z

Size increase report for "esp32-example-build" from 0e19e0d

File	Section	File	VM
chip-temperature-measurement-app.elf	.flash.text	52	52
chip-lock-app.elf	.flash.text	60	60
chip-all-clusters-app.elf	.flash.text	-4	-4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.flash.text,52,52
.debug_str,0,2
.debug_line,0,-10
.debug_info,0,-12
[Unmapped],0,-52
.debug_loc,0,-100

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

sections,vmsize,filesize
.flash.text,60,60
.debug_info,0,-12
.debug_line,0,-28
[Unmapped],0,-60
.debug_loc,0,-100

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

sections,vmsize,filesize

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

sections,vmsize,filesize

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize
[Unmapped],0,4
.flash.text,-4,-4
.debug_info,0,-12
.debug_line,0,-28
.debug_loc,0,-100

bzbarsky-apple · 2021-07-12T21:28:10Z

Yes, we should add more tests between different apps -- I would suggest a follow up.

I respectfully disagree, unless the tests are a lot of work to add (which it doesn't seem like they should be if all-clusters-app shows the problem too). This is fixing an app-visible bug, we have an existing harness for exercising such things, it's easy to add tests to this harness, tests should be added with the fix if possible. Why would we not add the tests here?

bzbarsky-apple · 2021-07-12T21:41:11Z

which it doesn't seem like they should be if all-clusters-app shows the problem too

Ah, but it does not, because ATTRIBUTE_LARGEST there is not 257 like it is in lighting-app; it's 1001, which leads to different behavior inside emberAfCopyString.

Alright, that's a good enough reason to sort out tests in a followup....

src/app/util/attribute-storage.cpp

src/app/util/util.cpp

* [fix] emberAfCopyString should accept size_t size * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]>

…8241) * [fix] emberAfCopyString should accept size_t size * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]>

boring-cyborg bot added the app label Jul 9, 2021

pullapprove bot requested review from andy31415, bzbarsky-apple, chrisdecenzo, Damian-Nordic, hawk248, jepenven-silabs and msandstedt July 9, 2021 05:54

pullapprove bot added the review - pending label Jul 9, 2021

erjiaqing force-pushed the fix/emberAfCopyString_u16 branch from 4f64c89 to 0ccdc0f Compare July 9, 2021 07:19

msandstedt approved these changes Jul 9, 2021

View reviewed changes

yunhanw-google added the TE4 cherrypick label Jul 9, 2021

yunhanw-google approved these changes Jul 9, 2021

View reviewed changes

bzbarsky-apple requested changes Jul 9, 2021

View reviewed changes

andy31415 approved these changes Jul 9, 2021

View reviewed changes

franck-apple removed the TE4 cherrypick label Jul 9, 2021

[fix] emberAfCopyString should accept size_t size

16d2700

erjiaqing force-pushed the fix/emberAfCopyString_u16 branch from 0ccdc0f to 16d2700 Compare July 12, 2021 01:58

erjiaqing changed the title ~~[fix] emberAfCopyString should accept uint16 size~~ [fix] emberAfCopyString should accept size_t size Jul 12, 2021

erjiaqing requested a review from bzbarsky-apple July 12, 2021 02:01

yunhanw-google changed the title ~~[fix] emberAfCopyString should accept size_t size~~ [fix] emberAfCopyString should accept size_t size[TE4] Jul 12, 2021

yunhanw-google added the TE4 cherrypick label Jul 12, 2021

yunhanw-google mentioned this pull request Jul 12, 2021

ZCLread Basic Attributes returning empty Value or not a valid AttributeValue #8273

Closed

bzbarsky-apple approved these changes Jul 12, 2021

View reviewed changes

src/app/util/attribute-storage.cpp Outdated Show resolved Hide resolved

src/app/util/attribute-storage.cpp Outdated Show resolved Hide resolved

src/app/util/util.cpp Outdated Show resolved Hide resolved

src/app/util/util.cpp Outdated Show resolved Hide resolved

pullapprove bot added review - approved and removed review - pending labels Jul 12, 2021

Apply suggestions from code review

ea42505

bzbarsky-apple merged commit a55a0aa into project-chip:master Jul 12, 2021

franck-apple removed the TE4 cherrypick label Jul 14, 2021

andy31415 pushed a commit that referenced this pull request Jul 15, 2021

[fix] emberAfCopyString should accept size_t size[TE4] (#8241)

c853050

* [fix] emberAfCopyString should accept size_t size * Apply suggestions from code review Co-authored-by: Boris Zbarsky <[email protected]>

andy31415 added the TE4 cherrypick DONE label Jul 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] emberAfCopyString should accept size_t size[TE4] #8241

[fix] emberAfCopyString should accept size_t size[TE4] #8241

erjiaqing commented Jul 9, 2021 •

edited

Loading

bzbarsky-apple left a comment

andy31415 commented Jul 9, 2021

msandstedt commented Jul 9, 2021

bzbarsky-apple commented Jul 9, 2021 •

edited

Loading

bzbarsky-apple commented Jul 9, 2021

franck-apple commented Jul 9, 2021

erjiaqing commented Jul 12, 2021

github-actions bot commented Jul 12, 2021

github-actions bot commented Jul 12, 2021

bzbarsky-apple commented Jul 12, 2021

bzbarsky-apple commented Jul 12, 2021

[fix] emberAfCopyString should accept size_t size[TE4] #8241

[fix] emberAfCopyString should accept size_t size[TE4] #8241

Conversation

erjiaqing commented Jul 9, 2021 • edited Loading

Problem

Change overview

Testing

bzbarsky-apple left a comment

Choose a reason for hiding this comment

andy31415 commented Jul 9, 2021

msandstedt commented Jul 9, 2021

bzbarsky-apple commented Jul 9, 2021 • edited Loading

bzbarsky-apple commented Jul 9, 2021

franck-apple commented Jul 9, 2021

erjiaqing commented Jul 12, 2021

github-actions bot commented Jul 12, 2021

github-actions bot commented Jul 12, 2021

bzbarsky-apple commented Jul 12, 2021

bzbarsky-apple commented Jul 12, 2021

erjiaqing commented Jul 9, 2021 •

edited

Loading

bzbarsky-apple commented Jul 9, 2021 •

edited

Loading