[CHIPTOOL] Fix AddArgument for ByteSpan when content is hexstring #38838
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
kliffwong
force-pushed
the
mr/fix-chiptool-bytespan
branch
from
aeb453c to
d9330a2
Compare
|
PR #38838: Size comparison from 1f6aada to d9330a2 Full report (46 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
kliffwong
force-pushed
the
mr/fix-chiptool-bytespan
branch
from
d9330a2 to
9041fa4
Compare
|
PR #38838: Size comparison from 1f6aada to 9041fa4 Full report (3 builds for cc32xx, stm32)
|
|
PR #38838: Size comparison from 1f6aada to 6f84ec7 Full report (75 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
requested changes
May 12, 2025
|
|
||
| if (err != CHIP_NO_ERROR) | ||
| { | ||
| if (buffer != nullptr) |
|
|
||
| arg.type = ArgumentType::OctetString; | ||
| arg.name = name; | ||
| arg.value = reinterpret_cast<void *>(&newValue); |
There was a problem hiding this comment.
You are passing in a void pointer to a value on the stack. That cannot be good. How was this tested?
|
@kliffwong the statement Since this is in test code (i.e. chip tool) it seems generally ok to just have manual test, but it does have to be described in detail. |
|
PR #38838: Size comparison from d0ac18d to 1afec32 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
kliffwong
changed the title
kliffwong
force-pushed
the
mr/fix-chiptool-bytespan
branch
from
1afec32 to
a3c5850
Compare
|
PR #38838: Size comparison from df611f5 to a3c5850 Full report (70 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, psoc6, qpg, stm32, telink, tizen)
|
kliffwong
force-pushed
the
mr/fix-chiptool-bytespan
branch
2 times, most recently
from
8238c8b to
07e6698
Compare
|
PR #38838: Size comparison from 6667bf2 to 07e6698 Full report (27 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, nrfconnect, qpg, stm32, telink, tizen)
|
kliffwong
force-pushed
the
mr/fix-chiptool-bytespan
branch
from
07e6698 to
dfdd368
Compare
bzbarsky-apple
requested changes
May 20, 2025
There was a problem hiding this comment.
Looking at this PR description, this bit:
./chip-tool thermostat set-active-preset-request hex:<Handle> <nodeid> <endpoint>
seems to work fine for me: if I pass hex:aabb12 as the first arg, those are the bytes that the server gets. So I am not sure what this is trying to fix exactly or what sorts of "manual testing" were were done.
| @@ -710,6 +710,37 @@ size_t Command::AddArgument(const char * name, chip::CharSpan * value, const cha | |||
| size_t Command::AddArgument(const char * name, chip::ByteSpan * value, const char * desc, uint8_t flags) | |||
| { | |||
| Argument arg; | |||
|
|
|||
| // if the value is a hex string, we need to convert it from hex to bytes | |||
| if ((value->size() > kHexStringPrefixLen) && IsHexString((const char *) value->data())) | |||
There was a problem hiding this comment.
This makes no sense. The way this works is that AddArgument registers a memory location where the value will live later when we have a value. At the point where this is called, value->size() is 0 at best (if it's a non-optional argument), and garbage at worst, because value might in fact be a reinterpret_cast pointer to an Optional<ByteSpan> here (if the kOptional flag is set).
The hex string detection should (and does) happen during the actual argument decoding, which is what will fill in the ByteSpan here. In particular, this will happen in Command::InitArgument.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing
./chip-tool thermostat set-active-preset-request hex:<Handle> <nodeid> <endpoint>./chip-tool thermostat set-active-schedule-request hex:<Handle> <nodeid> <endpoint><Handle>as String to the node, which does not comply with the format used in./chip-tool thermostat write presets/schedules <arg> <nodeid> <endpoint><Handle>and already parse the<Handle>from HexString to number with this fix