Skip to content

nrf802154: don't call memcpy if iolist->iol_len==0#11176

Merged
1 commit merged intoRIOT-OS:masterfrom
bergzand:pr/nrf802154/undef_memcpy
Mar 25, 2019
Merged

nrf802154: don't call memcpy if iolist->iol_len==0#11176
1 commit merged intoRIOT-OS:masterfrom
bergzand:pr/nrf802154/undef_memcpy

Conversation

@bergzand
Copy link
Copy Markdown
Member

@bergzand bergzand commented Mar 13, 2019 •
edited
Loading

Contribution description

Should fix an ubsan complaint as memcpy with iol->iol_base==NULL is undefined.

Testing procedure

Preferably with the procedure lined out in #11163

Issues/PRs references

related: #10782 and #11163

@bergzand bergzand added Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: drivers Area: Device drivers labels Mar 13, 2019
@bergzand bergzand requested review from a user, kaspar030 and miri64 March 13, 2019 20:17
@bergzand bergzand added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR and removed CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Mar 13, 2019
@miri64 miri64 mentioned this pull request Mar 13, 2019
Closed
24 tasks
miri64
miri64 reviewed Mar 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

LGTM

@miri64 miri64 added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Mar 13, 2019
@bergzand bergzand added this to the Release 2019.04 milestone Mar 14, 2019
@ghost
Copy link
Copy Markdown

ghost commented Mar 15, 2019

I dont really get the necessity for this PR. Afaik memcpy is able to handle size zero and len will not increase either. Am I wrong?

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Mar 15, 2019

Afaik memcpy is able to handle size zero

The issue here is not that the len==0, but more that the iol->iol_base == NULL (which is the case when iol->iol_len==0), I've updated the description above, sorry for the confusion.

len will not increase either.

This is just a really small runtime optimization from my side, didn't affect the size of the final binary.

@ghost
Copy link
Copy Markdown

ghost commented Mar 15, 2019

The issue here is not that the len==0, but more that the iol->iol_base == NULL (which is the case when iol->iol_len==0), I've updated the description above, sorry for the confusion.

Wouldn't it be smarter to test iol->iol_base != NULL then?

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Mar 15, 2019

Wouldn't it be smarter to test iol->iol_base != NULL then?

You're probably right…, will update

@ghost
Copy link
Copy Markdown

ghost commented Mar 15, 2019

So uhm, iol->iol_len==0 should be a correct test for the problem (as long as it's set correctly, corresponding to iol_base, but let's assume that ^^). Also memcpy does really not have to be executed when iol->iol_len==0.

What I want to say is that the fixup is not necessary - testing on length should be sufficient as it was. But i'd like a comment then, so if anyone reads this, the reasoning behind this check is clear on first sight.

@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Mar 15, 2019

What I want to say is that the fixup is not necessary - testing on length should be sufficient as it was. But i'd like a comment then, so if anyone reads this, the reasoning behind this check is clear on first sight.

Whahaha! 😆
So, revert the fixup and add a comment here to clarify?

@ghost
Copy link
Copy Markdown

ghost commented Mar 15, 2019

So, revert the fixup and add a comment here to clarify?

Thats what I would do :D

@ghost ghost added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Mar 15, 2019
@bergzand bergzand force-pushed the pr/nrf802154/undef_memcpy branch from 6187f5b to e756bf2 Compare March 15, 2019 16:19
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Mar 15, 2019

@SemjonKerner Added a new fixup commit that clarifies the conditional a bit.

@ghost
Copy link
Copy Markdown

ghost commented Mar 22, 2019

As expect this PR works with and without the fix as intended.
Please squash.

@ghost ghost added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Mar 22, 2019
@bergzand bergzand force-pushed the pr/nrf802154/undef_memcpy branch from e756bf2 to b4bb144 Compare March 22, 2019 18:01
@bergzand
Copy link
Copy Markdown
Member Author

bergzand commented Mar 22, 2019

Squashed, thanks for the review!

@ghost
Copy link
Copy Markdown

ghost commented Mar 25, 2019

Thank you for your contribution :)

@ghost ghost merged commit 253cf0f into RIOT-OS:master Mar 25, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

@kaspar030 kaspar030 Awaiting requested review from kaspar030

+1 more reviewer
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Milestone

Release 2019.04

Development

Successfully merging this pull request may close these issues.

2 participants

@bergzand @miri64