Fix casting in TLVReader::Get to be safe. #3099
Conversation
pullapprove
bot
requested review from
andy31415,
BroderickCarlin,
chrisdecenzo,
hawk248,
jelderton,
mspang,
rwalker-apple and
saurabhst
bzbarsky-apple
force-pushed
the
better-signed-casts
branch
from
3986242
to
befc11c
Compare
|
Do we need to do this if c++ 20 will standardize two's complement representation for signed integers which is already de facto standard? [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r1.html Edit: oops, wrong link |
There was a problem hiding this comment.
We should add some unit tests for this TLVReader ... if the functions were unsafe/not working, hopefully unit tests would have caught that. Or if working by chance, unit tests would have shown that changes did not affect functionality.
Approving as unit tests could be done as a followup, however I believe we should soon.
| * return an int8_t with value -2. | ||
| */ | ||
| template <typename T> | ||
| typename std::enable_if<std::is_unsigned<T>::value, typename std::make_signed<T>::type>::type CastToSigned(T arg) |
There was a problem hiding this comment.
Could we have some unit tests for this? They would serve as examples as well.
There was a problem hiding this comment.
Yes, will add tests.
|
Could you also rebase to make sure bloatcheck sees the impact on code size? Hopefully there is none. |
If this happens, then yes, the behavior here will be well-defined and we would not need this function. |
bzbarsky-apple
force-pushed
the
better-signed-casts
branch
from
befc11c
to
0c8e268
Compare
Casts of out-of-range values to signed integer types are not defined by the C or C++ standards, but this code was expecting them to act as a cast to the corresponding-sized unsigned integer type, then the "undoing" of a signed-to-unsigned cast. We instead introduce a CastToSigned() function that explicitly and safely undoes a signed-to-unsigned cast and use that in TLVReader::Get to eliminate the implementation-defined behavior dependence.
bzbarsky-apple
force-pushed
the
better-signed-casts
branch
from
0c8e268
to
2be17d3
Compare
|
Fwiw, we do have |
|
As far as bloat check goes, https://github.com/project-chip/connectedhomeip/runs/1221097449?check_suite_focus=true says no changes for all the artifacts for this PR. I'm not sure whether we should try to change the bloat check script to add a single "no bloat changes" comment or something.... |
Did not see them ... was searching for TestTLVReader which matches the pattern I saw for other unit tests. Maybe low priority task to rename things to be consistent. I feel better that we actually have tests. |
|
Yeah, the naming is interesting. |
* Use enum class for TLVElementType and TLVTagControl. This makes it easy to give them as specific size spec, which will prevent issues with people adding out-of-range values and simplify -Wconversion work. * Check if the result of EncodeCommand is successful before trying to send the command (#3151) * Remove verbose code comments (#3187) Some devices are configured to act as Thread router instead of sleepy-end device. This PR removes these incorrect or verbose comments. * Remove {#mainpage} from the main README.md file (#3190) * Remove Makefiles introduced by #3099 (#3191) * [nRF Connect] README updates on Docker for MacOS and NCS version (#3175) * [nRF Connect] README updates on Docker for MacOS and NCS version * Restyled by prettier-markdown * Rephrase a sentence Co-authored-by: Restyled.io <commits@restyled.io> * Modify the generated files for example lighting and lock apps to include server init callbacks. (#3180) This allows the app to implement emberAfPluginOnOffClusterServerPostInitCallback to sync up the data model state with the state of the actual device when the data model initializes. * Use pragma once in more places (#3182) * Add a lot of pragma once changes. Now scripted! * update pragma once in setup payload * More pragma once * Pragma once within platform and more * pragma once in the crypto layer * pragma once in examples * Fix by restyle-diff * [android] Use ATT Write Request instead of ATT Write Command (#3161) Co-authored-by: Vivien Nicolas <vnicolas@apple.com> Co-authored-by: Yakun Xu <xyk@google.com> Co-authored-by: Damian Królik <66667989+Damian-Nordic@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: Andrei Litvin <andrei@andy314.com> Co-authored-by: Łukasz Duda <lukasz.duda@nordicsemi.no>
Casts of out-of-range values to signed integer types are not defined
by the C or C++ standards, but this code was expecting them to act as
a cast to the corresponding-sized unsigned integer type, then the
"undoing" of a signed-to-unsigned cast.
We instead introduce a CastToSigned() function that explicitly and
safely undoes a signed-to-unsigned cast and use that in TLVReader::Get
to eliminate the implementation-defined behavior dependence.
Problem
If you have a
uint64_twith value255, casting it toint8_tis not guaranteed to produce-1, but theTLVReadercode depends on this behavior.Summary of Changes
Replace the implementation-dependent casting with a safe function that produces the desired results.