Add a buffer reader helper class that can be used to read buffers safely. #3471

bzbarsky-apple · 2020-10-27T15:21:29Z

The idea is to not have people doing ad-hoc, and often incorrect when
they forget to update lengths, length checks.

I did measure the codesize with the Read methods out-of-line, and it's
larger than with the inline methods as far as I can tell. It might be
worth re-measuring once we have more callers to see what things look
like then.

Problem

It's too easy to forget to update available length when reading things out of buffers and hence end up with potential buffer overruns.

Summary of Changes

Add a class that encapsulates the updating of the read pointer and the updating of the available length in one place in a way that guarantees they stay in sync.

Fixes #2914

One open question: Do we still want the VerifyOrExit calls that verify that octets_read matches some value? It's not clear that we do: they used to be there to ensure the number we were about to cast to uint16_t would in fact fit in uint16_t, but that's no longer in question... Is there another real use for those checks?

gerickson

Minor API nit; however, otherwise, seems reasonable.

gerickson · 2020-10-27T15:26:49Z

src/lib/support/BufferReader.h

+     *         enough octets available.
+     */
+    CHECK_RETURN_VALUE
+    CHIP_ERROR ReadOctet(uint8_t * octet) { return Read(octet); }


Why not Read8? This seems to be an odd API nomenclature exception.

Good catch. This started out as a different class with more-semantic function names, and I forgot to rename this one. Will fix.

gerickson · 2020-10-27T15:29:56Z

src/lib/support/BufferReader.h

+ *
+ *  Simple reader for reading little-endian things out of buffers.
+ */
+class LittleEndianReader


I'd recommend following both nlio and CHIPEncoding convention and create both LittleEndian and BigEndian namespaces with a reader class instance in each.

Also good catch. I'm going to do LittleEndian::Reader and not worry about the BigEndian version for now. If we ever need that (which I doubt), we can add it easily.

So the issue here is that once I have chip::LittleEndian::Reader and chip::Encoding::LittleEndian::*, LittleEndian becomes ambiguous and more full qualification is needed. I guess I can do chip::Encoding::LittleEndian::Reader... @gerickson any preferences?

I like chip::Encoding::LittleEndian::Reader.

I guess that ambiguity was because I was in code that had using namespace chip::Encoding.

github-actions · 2020-10-27T15:46:12Z

Size increase report for "nrfconnect-example-build" from 3b333f0

File	Section	File	VM
chip-lighting.elf	text	168	168
chip-lighting.elf	shell_root_cmds_sections	-8	-8
chip-lock.elf	text	168	168
chip-lock.elf	shell_root_cmds_sections	-8	-8

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-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,2196
.debug_loc,0,1672
.debug_str,0,1024
.debug_ranges,0,264
.debug_line,0,234
text,168,168
.debug_abbrev,0,126
.strtab,0,84
.symtab,0,64
.debug_frame,0,60
.debug_aranges,0,16
shell_root_cmds_sections,-8,-8

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

sections,vmsize,filesize
.debug_info,0,2196
.debug_loc,0,1676
.debug_str,0,1024
.debug_ranges,0,264
.debug_line,0,238
text,168,168
.debug_abbrev,0,126
.strtab,0,84
.symtab,0,64
.debug_frame,0,60
.debug_aranges,0,16
shell_root_cmds_sections,-8,-8

github-actions · 2020-10-27T16:15:03Z

Size increase report for "esp32-example-build" from 3b333f0

File	Section	File	VM
chip-wifi-echo.elf	.flash.text	72	72

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_info,0,2047
.debug_loc,0,1370
.debug_str,0,1029
.debug_line,0,585
.shstrtab,0,202
.xt.prop._ZN4chip18LittleEndianReader4ReadItEEiPT_,0,168
.debug_ranges,0,144
.xt.prop._ZN4chip18LittleEndianReader4ReadIyEEiPT_,0,88
.debug_abbrev,0,87
.symtab,0,80
.strtab,0,74
.flash.text,72,72
.debug_frame,0,24
.debug_aranges,0,8
.xt.prop._ZN4chip8Encoding12LittleEndian7Write32ERPhj,0,-2

rwalker-apple

man, this stuff is crying out for a sscanf()-like facility

github-actions · 2020-10-27T17:12:37Z

Size increase report for "nrfconnect-example-build" from 7257d44

File	Section	File	VM
chip-lock.elf	text	168	168
chip-lock.elf	shell_root_cmds_sections	-8	-8
chip-lighting.elf	text	168	168
chip-lighting.elf	shell_root_cmds_sections	8	8

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_info,0,2199
.debug_loc,0,1677
.debug_str,0,1175
.debug_ranges,0,264
.debug_line,0,233
text,168,168
.debug_abbrev,0,112
.strtab,0,104
.symtab,0,64
.debug_frame,0,60
.debug_aranges,0,16
shell_root_cmds_sections,-8,-8

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

sections,vmsize,filesize
.debug_info,0,2199
.debug_loc,0,1677
.debug_str,0,1175
.debug_ranges,0,264
.debug_line,0,233
text,168,168
.debug_abbrev,0,112
.strtab,0,104
.symtab,0,64
.debug_frame,0,60
.debug_aranges,0,16
shell_root_cmds_sections,8,8

github-actions · 2020-10-27T17:38:11Z

Size increase report for "esp32-example-build" from 7257d44

File	Section	File	VM
chip-wifi-echo.elf	.flash.text	72	72

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_info,0,2050
.debug_loc,0,1373
.debug_str,0,1186
.debug_line,0,585
.shstrtab,0,242
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadItEEiPT_,0,168
.debug_ranges,0,144
.strtab,0,94
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadIyEEiPT_,0,88
.symtab,0,80
.debug_abbrev,0,73
.flash.text,72,72
.debug_frame,0,24
.debug_aranges,0,8
.xt.prop._ZN4chip8Encoding12LittleEndian7Write32ERPhj,0,1

…ely. The idea is to not have people doing ad-hoc, and often incorrect when they forget to update lengths, length checks. I did measure the codesize with the Read methods out-of-line, and it's larger than with the inline methods as far as I can tell. It might be worth re-measuring once we have more callers to see what things look like then.

github-actions · 2020-10-27T18:38:33Z

Size increase report for "nrfconnect-example-build" from 2ea6342

File	Section	File	VM
chip-lock.elf	text	168	168
chip-lock.elf	shell_root_cmds_sections	-8	-8
chip-lighting.elf	text	168	168
chip-lighting.elf	shell_root_cmds_sections	8	8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,2199
.debug_loc,0,1673
.debug_str,0,1175
.debug_ranges,0,264
.debug_line,0,237
text,168,168
.debug_abbrev,0,112
.strtab,0,104
.symtab,0,64
.debug_frame,0,60
.debug_aranges,0,16
shell_root_cmds_sections,-8,-8

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

sections,vmsize,filesize
.debug_info,0,2199
.debug_loc,0,1677
.debug_str,0,1175
.debug_ranges,0,264
.debug_line,0,233
text,168,168
.debug_abbrev,0,112
.strtab,0,104
.symtab,0,64
.debug_frame,0,60
.debug_aranges,0,16
shell_root_cmds_sections,8,8

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

sections,vmsize,filesize

andy31415 · 2020-10-27T18:47:00Z

src/lib/support/BufferReader.h

+
+        static constexpr size_t data_size = sizeof(T);
+
+        if (mAvailable < data_size)


thought: should we somehow try to make errors persistent? If I have (wrong really) code like:

Reader reader(buffer, 2); reader.Read32(&foo); return reader.Read16(&bar);

we do not notice that a value failed to be read in between. I imagine we generally want to either use some GetLength() call to figure out available length (if data is variable size) or once reader failed to read, it became invalid.

If we have persistent errors, we can have builder patters:

Reader reader(buffer, len); return reader.Read8(&foo).Read16(&bar).Read8(&baz).StatusCode();

instead of a more verbose VerifyOrExit:

CHIP_ERROR err = CHIP_NO_ERROR; Reader reader(buffer, len); err = reader.Read8(&foo); SuccessOrExit(err); err = reader.Read16(&bar); SuccessOrExit(err); err = reader.Read8(&baz); SuccessOrExit(err); exit: return err;

I could have similar SuccessOrExit(reader.StatusCode()) if I wanted to for some performance benefits, however I generally expect happy path to almost alwyas happen and would be willing to trade some ifs cost on failure for smaller flash usage.

If I have (wrong really) code like

That code won't compile, because Read32 is marked as CHECK_RETURN_VALUE and in this code that return value is being ignored.

You could have code like this, though:

CHIP_RESULT res; Reader reader(buffer, 2); res = reader.Read32(&foo); return reader.Read16(&bar);

but hopefully at that point you would notice that you are not doing anything with res. And even better would be if we could make the compiler notice...

If we have persistent errors, we can have builder patterns:

That does make sense, in cases when all the reads are unconditional. In cases when the reads are conditional on the value a previous read returned, people would need to make sure to examine the success value of the previous read. I guess we could maybe do that if we require that the reference we return will be used (for either an immediate unconditional read call or for an error check)?

One other note: I plan to build a data-model-specific reader on top of this one to address #2168. But I suspect that might also be possible with the reader pattern. Will check.

I'll prototype out this suggestion and see how the code and the resulting codesize look.

@andy31415 OK, I put up some example code at https://github.com/bzbarsky-apple/connectedhomeip/tree/little-endian-reader-builder and https://github.com/bzbarsky-apple/connectedhomeip/tree/data-model-message-reader-builder for the builder pattern approach. It seems to use more codesize than the code I had before, though maybe it would do a bit better with some more out-of-lining, since Read ends up with a bit more logic.

Fundamentally, the builder setup still ends up doing the "did the previous call succeed" on every read check, which is what the SuccessOrExit was doing...

I did test instead setting mAvailable to 0 on read failure, so we don't have the extra branch on the status, but the end result is the same in terms of codesize; presumably the assignment is about the same size as the status-check branch.

We can iterate. I would expect it to be smaller in size, but only if used a lot (so that the save of no extra if is worth it to extra logic inside reads).

I am ok with the PR as is.

OK, that works. I'm pretty happy to revisit once we have more consumers and can get better size data. Converting should not be that big a problem, I suspect....

I did some more experimenting, and I think you're right and the builder pattern might be a better fit here. Filed #3495 and will put up a PR soon.

github-actions · 2020-10-27T18:56:02Z

Size increase report for "esp32-example-build" from 2ea6342

File	Section	File	VM
chip-wifi-echo.elf	.flash.text	72	72

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_info,0,2050
.debug_loc,0,1381
.debug_str,0,1186
.debug_line,0,585
.shstrtab,0,242
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadItEEiPT_,0,168
.debug_ranges,0,144
.strtab,0,94
.xt.prop._ZN4chip8Encoding12LittleEndian6Reader4ReadIyEEiPT_,0,88
.symtab,0,80
.debug_abbrev,0,73
.flash.text,72,72
.debug_frame,0,24
.debug_aranges,0,8
.xt.prop._ZN4chip8Encoding12LittleEndian7Write32ERPhj,0,1

bzbarsky-apple · 2020-10-27T21:05:40Z

@saurabhst @BroderickCarlin @jelderton

bzbarsky-apple requested review from andy31415 and pan-apple October 27, 2020 15:21

boring-cyborg bot added lib transport labels Oct 27, 2020

pullapprove bot requested review from BroderickCarlin, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple and saurabhst October 27, 2020 15:21

gerickson approved these changes Oct 27, 2020

View reviewed changes

gerickson reviewed Oct 27, 2020

View reviewed changes

rwalker-apple approved these changes Oct 27, 2020

View reviewed changes

bzbarsky-apple force-pushed the little-endian-reader branch from 2035a93 to ef473be Compare October 27, 2020 16:56

bzbarsky-apple force-pushed the little-endian-reader branch from ef473be to f63ccce Compare October 27, 2020 18:20

andy31415 approved these changes Oct 27, 2020

View reviewed changes

saurabhst approved these changes Oct 27, 2020

View reviewed changes

bzbarsky-apple merged commit c82c324 into project-chip:master Oct 27, 2020

bzbarsky-apple deleted the little-endian-reader branch October 27, 2020 21:19

bzbarsky-apple mentioned this pull request Oct 28, 2020

Switch to builder pattern for buffer reader #3495

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a buffer reader helper class that can be used to read buffers safely. #3471

Add a buffer reader helper class that can be used to read buffers safely. #3471

bzbarsky-apple commented Oct 27, 2020

gerickson left a comment

gerickson Oct 27, 2020

bzbarsky-apple Oct 27, 2020

gerickson Oct 27, 2020

bzbarsky-apple Oct 27, 2020

bzbarsky-apple Oct 27, 2020

gerickson Oct 27, 2020

bzbarsky-apple Oct 27, 2020

github-actions bot commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

rwalker-apple left a comment

github-actions bot commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

andy31415 Oct 27, 2020 •

edited

Loading

bzbarsky-apple Oct 27, 2020

bzbarsky-apple Oct 27, 2020

andy31415 Oct 27, 2020

bzbarsky-apple Oct 27, 2020

bzbarsky-apple Oct 28, 2020

github-actions bot commented Oct 27, 2020

bzbarsky-apple commented Oct 27, 2020


		static constexpr size_t data_size = sizeof(T);

		if (mAvailable < data_size)

Add a buffer reader helper class that can be used to read buffers safely. #3471

Add a buffer reader helper class that can be used to read buffers safely. #3471

Conversation

bzbarsky-apple commented Oct 27, 2020

Problem

Summary of Changes

gerickson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

rwalker-apple left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

github-actions bot commented Oct 27, 2020

andy31415 Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 27, 2020

bzbarsky-apple commented Oct 27, 2020

andy31415 Oct 27, 2020 •

edited

Loading