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.

Sign up for GitHub

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

Jump to bottom

net: sockets: socketpair: fix socketpair coverity issues #25805

Merged
merged 8 commits into from
Jun 3, 2020
Merged

net: sockets: socketpair: fix socketpair coverity issues #25805

merged 8 commits into from
Jun 3, 2020

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented May 28, 2020 •
edited
Loading

I have no permissions to access the actual errors on coverity.com (error 401), but please let me know if this helps.

Fixes #25793
Fixes #25788
Fixes #25780
Fixes #25737
Fixes #25731
Fixes #25736
Fixes #25727
Fixes #25797

@zephyrbot zephyrbot added area: Networking area: Samples Samples area: Tests Issues related to a particular existing or missing test labels May 28, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented May 28, 2020 •
edited
Loading

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

jukkar
jukkar requested changes May 29, 2020
View reviewed changes
Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, they look good. We usually add the Coverity CID information to commits that fix these things.
Could you add to each commit this line before/after the "Fixes" line:

Coverity-CID: xxxx

so that we can find from git log whether a certain Coverity issue was fixed or not.

@carlescufi carlescufi added this to the v2.3.0 milestone May 29, 2020
@cfriedt cfriedt requested a review from jukkar May 29, 2020 14:26
@cfriedt
Copy link
Member Author

cfriedt commented May 29, 2020 •
edited
Loading

@jukkar Should be good now. Does Coverity mention when a problem is fixed on GitHub? I'll keep an eye on it in any case.

@jukkar
Copy link
Member

jukkar commented May 29, 2020

Does Coverity mention when a problem is fixed on GitHub?

Unfortunately no. I need to manually mark things fixed in Coverity.

@cfriedt
Copy link
Member Author

cfriedt commented May 31, 2020 •
edited
Loading

@pfalcon, @jukkar, @tbursztyka, @carlescufi - should be good to go

cfriedt added 8 commits May 31, 2020 11:06
@cfriedt
net: socket: socketpair: remove dead code
e1937aa 
In this, case is_nonblock is false and will_block is true.
Therefore, we *may* block, and furthermore we *expect* to
block. Checking is_nonblock is, in fact, redundant, and
passing K_FOREVER to k_sem_take() is justified.

Fixes #25727
Coverity-CID: 210611

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt
tests: socket: socketpair: assert recvfrom returns >= 0
5eced2b 
Fixes #25731
Coverity-CID: 210568

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt
samples: sockets: socketpair: check return value of send
dfdc567 
Fixes #25736
Coverity-CID: 210583

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt
samples: sockets: socketpair: check return of pthread_create
afe23f4 
Fixes #25737
Coverity-CID: 210585

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt
samples: sockets: socketpair: mitigate negative index
802e250 
Fixes #25780
Coverity-CID: 210612

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt
net: socket: socketpair: initialize after NULL check
192addf 
Fixes #25788
Coverity-CID: 210581

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt
tests: socket: socketpair: init param before passing by ref
e5d4d3a 
Fixes #25796
Coverity-CID: 210579

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt
tests: socket: socketpair: init param before passing by ref
07c6769 
Fixes #25797
Coverity-CID: 210607

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt cfriedt self-assigned this May 31, 2020
@carlescufi
Copy link
Member

@pfalcon, @jukkar, @tbursztyka, @carlescufi - should be good to go

Thanks! waiting for the reviewers above to approve

@carlescufi carlescufi added the Coverity A Coverity detected issue or its fix label Jun 1, 2020
pabigot
pabigot requested changes Jun 2, 2020
View reviewed changes
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

Some of the associations between changes and Coverity IDs don't make sense. Other comments need changes too; I think the ones that are optional note that.

subsys/net/lib/sockets/socketpair.c Show resolved Hide resolved
tests/net/socket/socketpair/src/test_socketpair_happy_path.c Outdated Show resolved Hide resolved
samples/net/sockets/socketpair/src/socketpair_example.c
@@ -47,7 +47,12 @@ static const char *const names[] = {
static void hello(int fd, const char *name)
{
/* write(2) should be used after #25443 */
send(fd, name, strlen(name), 0);
int res = send(fd, name, strlen(name), 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This checks the return value as Coverity wants, but it's not a good example IMO because surely the failure should be returned to the caller and checked there.

Not blocking, net maintainers should weigh in.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it's sample code, I'm mostly impartial here. I just thought it would might be useful to print an error message to notify the user who would, presumably, be watching.

samples/net/sockets/socketpair/src/socketpair_example.c Outdated Show resolved Hide resolved
samples/net/sockets/socketpair/src/socketpair_example.c Show resolved Hide resolved
tests/net/socket/socketpair/src/test_socketpair_unsupported_calls.c Outdated Show resolved Hide resolved
tests/net/socket/socketpair/src/test_socketpair_happy_path.c Show resolved Hide resolved
@pfalcon
Copy link
Contributor

pfalcon commented Jun 2, 2020

Some of the associations between changes and Coverity IDs don't make sense. Other comments need changes too; I think the ones that are optional note that.

Well, my primary attention is to the changes outside tests/ and samples/. Changes to those can be treated in more or less lax way, but for changes to the system functional code, would be nice to have clear and detailed commit messages.

@cfriedt
Copy link
Member Author

cfriedt commented Jun 3, 2020

Some of the associations between changes and Coverity IDs don't make sense. Other comments need changes too; I think the ones that are optional note that.

Well, my primary attention is to the changes outside tests/ and samples/. Changes to those can be treated in more or less lax way, but for changes to the system functional code, would be nice to have clear and detailed commit messages.

@pabigot, @pfalcon - I believe most of your concerns should be addressed. If there is anything else, please let me know.

@cfriedt cfriedt requested review from pfalcon and pabigot June 3, 2020 14:53
@pabigot pabigot mentioned this pull request Jun 3, 2020
Closed
@carlescufi carlescufi merged commit 79728a6 into zephyrproject-rtos:master Jun 3, 2020
@cfriedt cfriedt deleted the socketpair-coverity-issues branch December 8, 2020 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanosio stephanosio stephanosio left review comments

@jukkar jukkar jukkar approved these changes

@pabigot pabigot pabigot approved these changes

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@pfalcon pfalcon Awaiting requested review from pfalcon

Assignees

@cfriedt cfriedt

Labels
area: Networking area: Samples Samples area: Tests Issues related to a particular existing or missing test Coverity A Coverity detected issue or its fix
Projects
None yet
Milestone
v2.3.0
Development

Successfully merging this pull request may close these issues.

[Coverity CID :210612] Negative array index read in samples/net/sockets/socketpair/src/socketpair_example.c [Coverity CID :210585] Unchecked return value in samples/net/sockets/socketpair/src/socketpair_example.c [Coverity CID :210583] Unchecked return value from library in samples/net/sockets/socketpair/src/socketpair_example.c [Coverity CID :210568] Argument cannot be negative in tests/net/socket/socketpair/src/test_socketpair_happy_path.c [Coverity CID :210611] Logically dead code in subsys/net/lib/sockets/socketpair.c
[Coverity CID :210581] Dereference before null check in subsys/net/lib/sockets/socketpair.c [Coverity CID :210561] Resource leak in tests/net/socket/socketpair/src/test_socketpair_unsupported_calls.c [Coverity CID :210607] Uninitialized scalar variable in tests/net/socket/socketpair/src/test_socketpair_happy_path.c
7 participants
@cfriedt @zephyrbot @jukkar @carlescufi @pfalcon @pabigot @stephanosio