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

problem with implementation of sock_set_flag #19191

Closed
eyuval opened this issue Sep 16, 2019 · 5 comments · Fixed by #19212
Closed

problem with implementation of sock_set_flag #19191

eyuval opened this issue Sep 16, 2019 · 5 comments · Fixed by #19212
Assignees
Labels
area: Networking bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@eyuval
Copy link

eyuval commented Sep 16, 2019

I am using sockets in non-blocking mode, i.e. I set O_NONBLOCK flag for my listening socket and for all newly accepted sockets. The sock_set_flag function in subsys/net/lib/sockets/sockets_internal.h uses the low bits of the net_context user_data pointer to store the SOCK_EOF/SOCK_NONBLOCK flags. I encounter problems, e.g. in zsock_accepted_cb, because the user_data pointer is used without discarding the flag bits, resulting in a pointer, which is off from the true pointer by some bytes, dependent on the flags.
I am working in the native posix system:

CONFIG_BOARD="native_posix_64"
CONFIG_BOARD_NATIVE_POSIX_64BIT=y

@eyuval eyuval added the bug The issue is a bug, or the PR is fixing a bug label Sep 16, 2019
@jukkar jukkar self-assigned this Sep 16, 2019
@jukkar
Copy link
Member

jukkar commented Sep 16, 2019

I quickly went through the code and it was not clear to me where the issue is. Only place that I noticed setting ctx->user_data was in zsock_socket_internal() which is the socket() call that initializes the ctx->user_data to NULL. The socket_internal.h has sock_set_flag() and sock_get_flag() use the ctx->user_data as you mentioned. Then sendto() calls either net_context_send() or net_context_sendto() with ctx->user_data and in net_context.c:context_sendto() sets it to itself.

Could you elaborate what kind of issues you were seeing?

@eyuval
Copy link
Author

eyuval commented Sep 16, 2019

My scenario is as following:

  1. I create a listening tcp socket and set it to non-blocking mode.
  2. For debbuging I set a breakpoint at zsock_accepted_cb and connect to the listening socket.
  3. When the zsock_accepted_cb breakpoint is hit I see the following:
    • The zsock_accepted_cb is called from _tcp_syn_rcvd with the user_data of the listening context passed as user data. In zsock_accepted_cb, the user_data pointer is simply cast to the listener context. But because of the flag bits, the passed in user_data pointer is not equal to the listener context. In my scenario this results in a crash zsock_accepted_cb, when we try to put the new context to the accept_q.

@jukkar
Copy link
Member

jukkar commented Sep 17, 2019

Yep, got the issue, thanks for the analysis. I will investigate how to solve it.

@jukkar
Copy link
Member

jukkar commented Sep 17, 2019

@eyuval can you try the proposed PR? It worked in my tests but I would like to get confirmation from you too.

jukkar added a commit to jukkar/zephyr that referenced this issue Sep 17, 2019
Do not try to re-use net_context.user_data field as in many places
(like in accept) it is expected to contain pointer to net_context.
Storing the socket flags will corrupt the value. To simplify and
make things less error prone, use socket specific field in net_context
to store the socket flags.

Fixes zephyrproject-rtos#19191

Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
@jukkar jukkar added the priority: medium Medium impact/importance bug label Sep 17, 2019
@eyuval
Copy link
Author

eyuval commented Sep 18, 2019

I tested the patch and for me it fixes the issue. Thanks.

jukkar added a commit that referenced this issue Sep 18, 2019
Do not try to re-use net_context.user_data field as in many places
(like in accept) it is expected to contain pointer to net_context.
Storing the socket flags will corrupt the value. To simplify and
make things less error prone, use socket specific field in net_context
to store the socket flags.

Fixes #19191

Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
jukkar added a commit to jukkar/zephyr that referenced this issue Sep 19, 2019
Do not try to re-use net_context.user_data field as in many places
(like in accept) it is expected to contain pointer to net_context.
Storing the socket flags will corrupt the value. To simplify and
make things less error prone, use socket specific field in net_context
to store the socket flags.

Fixes zephyrproject-rtos#19191

Signed-off-by: Jukka Rissanen <jukka.rissanen@linux.intel.com>
nashif pushed a commit that referenced this issue Sep 25, 2019
Do not try to re-use net_context.user_data field as in many places
(like in accept) it is expected to contain pointer to net_context.
Storing the socket flags will corrupt the value. To simplify and
make things less error prone, use socket specific field in net_context
to store the socket flags.

Fixes #19191

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 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