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

Switch message header encode/decode input lengths to uint16_t. #3449

Merged
merged 1 commit into from
Oct 27, 2020

Conversation

bzbarsky-apple
Copy link
Contributor

This will remove some impedance mismatches around lengths later on.

Problem

Message header encode/decode functions take length as size_t, whereas everything else, pretty much, uses uint16_t.

Summary of Changes

Just use uint16_t like elsewhere.

This will remove some impedance mismatches around lengths later on.
@github-actions
Copy link

Size increase report for "esp32-example-build" from 960cfe0

File Section File VM
chip-wifi-echo.elf .flash.text 20 20
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

sections,vmsize,filesize
.debug_line,0,60
.flash.text,20,20
.debug_loc,0,12


@@ -266,7 +266,7 @@ void SecureSessionMgrBase::HandleDataReceived(const PacketHeader & packetHeader,
VerifyOrExit(
payloadlen <= len,
(ChipLogError(Inet, "Secure transport can't find MAC Tag; buffer too short"), err = CHIP_ERROR_INVALID_MESSAGE_LENGTH));
err = mac.Decode(packetHeader, &data[payloadlen], len - payloadlen, &taglen);
err = mac.Decode(packetHeader, &data[payloadlen], static_cast<uint16_t>(len - payloadlen), &taglen);
Copy link
Contributor

Choose a reason for hiding this comment

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

this kind of cast makes me wonder if we're going the wrong direction with this. maybe we should isolate the uint16_t-limitations in PacketBuffer and in DM protocol details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this kind of cast makes me wonder if we're going the wrong direction with this

The biggest problem here is that C's integer-promotion rules for arithmetic are kind of dumb. If those were unsigned int, the difference would also be unsigned int, but a difference of two uint16_t gets promoted to int. Without -Wconversion it's not an issue, of course, because the narrowing conversion is silent then... I wish compilers were smarter here; a sufficiently smart compiler can in fact prove that len - payloadlen is a valid uint16_t value based on those two lines of code, and could avoid warning about the "narrowing" conversion. Ah, well.

We could switch a bunch of this stuff to size_t, but then we run into the need to do a bunch of casting and/or dynamic checks needed when indexing into PacketBuffers.

I guess I could change the in-progress code I have for #2914 to take size_t-sized buffers, not uint16_t-sized, so we can keep accepting size_t-sized things in the Decode methods here. We get impedance mismatches that way too, though....

@andy31415 andy31415 merged commit f6157a2 into project-chip:master Oct 27, 2020
@bzbarsky-apple bzbarsky-apple deleted the header-size-uint16 branch October 27, 2020 14:56
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