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

Fix buffer overruns and endianness issues in decoder code. #2362

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

There are two issues being fixed here:

  1. The code was checking that it had at least one more byte available
    to read, but then reading multiple bytes in some cases.

  2. The code was reading the data (which is little-endian) via memcpy,
    which would not do the right thing on big-endian systems.

The former problem is fixed by checking that we can read the number of
bytes we plan to read; the latter problem is fixed by using
ChipEncoding to read the data explicitly as little-endian data.

fixes #1632

Copy link
Contributor

@rwalker-apple rwalker-apple left a comment

Choose a reason for hiding this comment

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

lgtm

There are two issues being fixed here:

1) The code was checking that it had at least one more byte available
to read, but then reading multiple bytes in some cases.

2) The code was reading the data (which is little-endian) via memcpy,
which would not do the right thing on big-endian systems.

The former problem is fixed by checking that we can read the number of
bytes we plan to read; the latter problem is fixed by using
ChipEncoding to read the data explicitly as little-endian data.
@bzbarsky-apple bzbarsky-apple force-pushed the fix-decoder-endianness branch from cabeff7 to decc859 Compare August 27, 2020 18:55
@github-actions
Copy link

Size increase report for "nrfconnect-example-build"

File Section File VM
chip-nrf52840-lock-example.elf text 44 44
chip-nrf52840-lock-example.elf [LOAD #1 [RWX]] 4 4
chip-nrf52840-lock-example.elf rodata -64 -64
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,902
.debug_frame,0,516
.debug_line,0,469
.debug_abbrev,0,256
.debug_aranges,0,112
.debug_ranges,0,112
.symtab,0,64
.debug_str,0,59
.strtab,0,54
text,44,44
[LOAD #1 [RWX]],4,4
[Unmapped],0,-16
rodata,-64,-64


@github-actions
Copy link

Size increase report for "esp32-example-build"

File Section File VM
chip-wifi-echo.elf .flash.text -40 -40
chip-wifi-echo.elf .flash.rodata -64 -64
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,895
.debug_line,0,434
.debug_frame,0,336
.debug_abbrev,0,277
.xt.lit._ZNK4chip8OptionalItE5ValueEv,0,200
.shstrtab,0,142
.debug_aranges,0,112
.debug_ranges,0,112
.symtab,0,96
.xt.prop._ZN2nl9ByteOrder18Swap16LittleToHostEt,0,80
.xt.prop._ZN6ReaderILi1EE4readERPKh,0,76
.xt.prop._ZN6ReaderILi2EE4readERPKh,0,76
[Unmapped],0,63
.debug_str,0,59
.strtab,0,54
.flash.text,-40,-40
.flash.rodata,-64,-64
.xt.lit._ZN4chip8Encoding5Read8ERPKh,0,-200


@github-actions
Copy link

Size increase report for "gn_nrf-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@github-actions
Copy link

Size increase report for "gn_linux-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@rwalker-apple
Copy link
Contributor

@rwalker-apple rwalker-apple merged commit 5d66326 into project-chip:master Aug 27, 2020
@bzbarsky-apple bzbarsky-apple deleted the fix-decoder-endianness branch August 28, 2020 04:00
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.

Make command encoder/decoder work regardless of hardware endianness
5 participants