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

tests: net: socket: socketpair: fails due to empty message header name #25925

Closed
ABOSTM opened this issue Jun 3, 2020 · 3 comments · Fixed by #25932
Closed

tests: net: socket: socketpair: fails due to empty message header name #25925

ABOSTM opened this issue Jun 3, 2020 · 3 comments · Fixed by #25932
Assignees
Labels
area: Networking area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Milestone

Comments

@ABOSTM
Copy link
Collaborator

ABOSTM commented Jun 3, 2020

Describe the bug
Running test tests/net/socket/socketpair/ fails.

To Reproduce
Steps to reproduce the behavior:

  1. west build -p -b nucleo_f429zi tests/net/socket/socketpair/
  2. west flash
  3. See error

Expected behavior
test passed

Screenshots or console output

Running test suite socketpair
===================================================================
starting test - test_socketpair_AF_LOCAL__SOCK_STREAM__0
E: syscall z_user_alloc_from_copy failed check: Memory region 0x00000000 (size 0) read access denied

    Assertion failed at WEST_TOPDIR/zephyr/tests/net/socket/socketpair/src/test_socketpair_happy_path.c:141: happy_path: (res equal to -1)
sendmsg(2) failed: 12
FAIL - test_socketpair_AF_LOCAL__SOCK_STREAM__0

Analysis
This is a regression introduced with PR #25706
Due to new check on messages parameters, test fails because in function happy_path() ,
msghdr.msg_name is not set and thus equal 0 (and the same for msg_namelen)

But in net_ip.h, in the structure comment, it seems this name is optional:

struct msghdr {
	void         *msg_name;       /* optional socket address */
	socklen_t     msg_namelen;    /* size of socket address */
...

So I don't know whether

  • test should be updated and structure comment ?

or

Note: probably the same will happend with msg_control field of the structure

@ABOSTM ABOSTM added bug The issue is a bug, or the PR is fixing a bug area: Tests Issues related to a particular existing or missing test area: Networking labels Jun 3, 2020
@ABOSTM
Copy link
Collaborator Author

ABOSTM commented Jun 3, 2020

^^ @jukkar

@jukkar
Copy link
Member

jukkar commented Jun 3, 2020

* PR #25706 should be reworked ?

As those couple of fields are optional for connected socket, the checks introduced in #25706 need to be fixed. Will send a fix asap.

@jukkar jukkar self-assigned this Jun 3, 2020
@jukkar jukkar added the priority: medium Medium impact/importance bug label Jun 3, 2020
@jukkar jukkar added this to the v2.3.0 milestone Jun 3, 2020
@jukkar
Copy link
Member

jukkar commented Jun 3, 2020

I just wonder why this issue was not caught by sanitychecker.

jukkar added a commit to jukkar/zephyr that referenced this issue Jun 3, 2020
If we are calling sendmsg() for a connected socket, then msg_namelen
is 0 and msg_name is NULL. Check these allowed values properly.

Also modify unit tests so that we test this scenario.

Fixes zephyrproject-rtos#25925

Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
carlescufi pushed a commit that referenced this issue Jun 3, 2020
If we are calling sendmsg() for a connected socket, then msg_namelen
is 0 and msg_name is NULL. Check these allowed values properly.

Also modify unit tests so that we test this scenario.

Fixes #25925

Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants