gnrc_sock: provide port for sock_ip and sock_udp by miri64 · Pull Request #5772 · RIOT-OS/RIOT

miri64 · 2016-08-26T09:35:35Z

This PR provides the port for sock_ip and sock_udp (see #5758) to GNRC + tests.

Depends on ~~#5526~~ (mbox support for GNRC), ~~#5749~~ (fix to UDP so the tests are able to run), and ~~#5758~~ (the sock header files).

miri64 · 2016-08-26T09:37:37Z

@brummer-simon this PR might also be of interest for you to give you an idea for how to port sock_tcp using mbox ;-).

miri64 · 2016-08-26T12:05:07Z

Rebased due to changes in #5526.

kaspar030 · 2016-09-27T21:18:44Z

sock is merged, pls rebase

miri64 · 2016-10-04T12:16:49Z

Can we merge the dependencies first please? Also, while working on lwIP I realized I need to adapt the tests a little ;-).

miri64 · 2016-10-04T13:41:20Z

Rebased to current dependencies and master.

miri64 · 2016-10-04T13:41:42Z

Applied the changes I spoke about too.

miri64 · 2016-10-04T16:36:34Z

Rebased to current master, since #5884 was merged.

miri64 · 2016-10-06T12:28:57Z

While working on #5802 I realized that their raw sock equivalent require the IP header to be provided. This seems to be the behavior with POSIX sockets, too (see e.g. this tutorial). I did not implement it like that in gnrc_sock_ip, because the API was conceptualized under a different premise (the header is build by the API). However that would make the implementation of the socket wrapper much more complicated. So how do we handle this?

miri64 · 2016-10-06T13:21:16Z

Nvm... lwIP isn't consistent in this itself... and so is the information about raw sockets themselves..

miri64 · 2016-10-12T16:01:58Z

Needs some small adjustments due to #5929 being merged.

haukepetersen · 2016-10-25T20:03:35Z

From my side everything looks in order. Tests succeed and I successfully tested the sock_udp implementation under native. ACK once squashed and Murdock is happy.

miri64 · 2016-10-25T20:05:16Z

Squashed

miri64 · 2016-10-25T21:05:35Z

Rebased to current master.

miri64 · 2016-10-25T23:08:49Z

Ooops, didn't adapt it to the pseudo-module change in #5526. Done now.

cgundogan · 2016-10-26T06:04:18Z

failed to download the tiny-asn1 pkg, retriggering murdock

cgundogan · 2016-10-26T07:21:58Z

Murdock still fails @miri64

miri64 · 2016-10-26T11:32:42Z

Fixed, rebased and squashed.

miri64 · 2016-10-26T13:26:51Z

Okay, cppcheck is really acting weird on this lines... first it complains about the "redundant" check (dereferencing sock after I checked if it is NULL in an || connection, which thanks to lazy evaluation should be fine), when I suppress it it complains that there is a NULL pointer dereference at that line (no it isn't because lazy evaluation) and when I write it out for it ((sock == NULL) || ((sock != NULL) && (sock->foobar == ...))) it is complaining that the additional check for (sock != NULL) isn't necessary, since (sock == NULL) || (sock->foobar == ...) is equivalent (which is what was there in the first place...) so I just suppressed everything on this line. Since my tests are covering this line and they are not crashing I guess we are fine and cppcheck just is weird. @cgundogan could you re-ACK please? The changes are in https://github.com/RIOT-OS/RIOT/pull/5772/files#diff-71ddba2942574ad9a8d73c8de31b2230R188 and https://github.com/RIOT-OS/RIOT/pull/5772/files#diff-97c7bd9404c4bc328009313d1665dfe5R150 respectively.

miri64 · 2016-10-26T14:00:59Z

Murdock is finally happy!

haukepetersen · 2016-10-27T09:43:40Z

I am good with the changes (so re-ACK) and Murdock is happy -> go

@miri64 Congrats, this is big step towards usability of GNRC!

sys/net/gnrc/sock/ip/gnrc_sock_ip.c

benpicco · 2022-11-06T14:03:06Z

sys/net/gnrc/sock/udp/gnrc_sock_udp.c

+    if ((sock->remote.family != AF_UNSPEC) &&  /* check remote end-point if set */
+        ((sock->remote.port != byteorder_ntohs(hdr->src_port)) ||
+        /* We only have IPv6 for now, so just comparing the whole end point
+         * should suffice */
+        ((memcmp(&sock->remote.addr, &ipv6_addr_unspecified,
+                 sizeof(ipv6_addr_t)) != 0) &&
+         (memcmp(&sock->remote.addr, &tmp.addr, sizeof(ipv6_addr_t)) != 0)))) {
+        gnrc_pktbuf_release(pkt);
+        return -EPROTO;
+    }


Why is this condition needed?
This enforces that if the socket was previously used for sending, received packets must come from the same address.

But there is nothing that says a socket must be bound to the remote address, but there is even a test for this.

Why is this there? This makes it impossible to use a socket to send to a multicast address and receive unicast responses.

miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT State: waiting for other PR State: The PR requires another PR to be merged first labels Aug 26, 2016

miri64 assigned kaspar030 Aug 26, 2016

miri64 mentioned this pull request Aug 26, 2016

sock: Introduction of new application layer API #5758

Merged

miri64 force-pushed the gnrc/feat/sock branch from b6f6ee1 to 1c48456 Compare August 26, 2016 12:04

miri64 added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 26, 2016

This was referenced Aug 30, 2016

tests: conn_ip: provide test scripts #4623

Closed

netif: initial import of a common network interface API #5511

Closed

dns: Review for DNS implementation #3823

Closed

dist: testrunner: optionally print traceback #5271

Merged

immesys mentioned this pull request Sep 29, 2016

sock: doc fixes #5884

Merged

miri64 added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 4, 2016

miri64 force-pushed the gnrc/feat/sock branch from 6a80b2d to 11fb938 Compare October 4, 2016 13:41

miri64 removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Oct 4, 2016

miri64 force-pushed the gnrc/feat/sock branch 3 times, most recently from 58c2df3 to 779ff21 Compare October 4, 2016 16:36

miri64 mentioned this pull request Oct 5, 2016

sock: define behavior on invalid addresses #5913

Merged

miri64 force-pushed the gnrc/feat/sock branch from e17852f to 63c6b58 Compare October 6, 2016 08:57

haukepetersen approved these changes Oct 25, 2016

View reviewed changes

miri64 force-pushed the gnrc/feat/sock branch from cd34cb9 to 149d29b Compare October 25, 2016 20:05

miri64 added this to the Release 2016.10 milestone Oct 25, 2016

miri64 removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable State: waiting for other PR State: The PR requires another PR to be merged first labels Oct 25, 2016

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 25, 2016

miri64 force-pushed the gnrc/feat/sock branch from 149d29b to 9818d55 Compare October 25, 2016 23:07

cgundogan added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 26, 2016

miri64 force-pushed the gnrc/feat/sock branch from 9818d55 to f3066f6 Compare October 26, 2016 11:32

miri64 and others added 4 commits October 26, 2016 15:20

gnrc_sock_udp: provide port for sock_udp

b971c57

gnrc_sock_ip: provide port for sock_ip

1a6ebf2

tests: provide gnrc_sock_udp tests

6bc0a0c

tests: provide gnrc_sock_ip tests

9508c8f

miri64 force-pushed the gnrc/feat/sock branch from f3066f6 to 9508c8f Compare October 26, 2016 13:20

haukepetersen merged commit f6413ee into RIOT-OS:master Oct 27, 2016

miri64 deleted the gnrc/feat/sock branch October 27, 2016 09:51

keestux reviewed Sep 5, 2020

View reviewed changes

sys/net/gnrc/sock/ip/gnrc_sock_ip.c Show resolved Hide resolved

miri64 mentioned this pull request Sep 7, 2020

gnrc_sock_ip: fix NULL pointer dereference #14963

Merged

benpicco reviewed Nov 6, 2022

View reviewed changes

Conversation

miri64 commented Aug 26, 2016 • edited by cgundogan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 26, 2016

Uh oh!

miri64 commented Aug 26, 2016

Uh oh!

kaspar030 commented Sep 27, 2016

Uh oh!

miri64 commented Oct 4, 2016

Uh oh!

miri64 commented Oct 4, 2016

Uh oh!

miri64 commented Oct 4, 2016

Uh oh!

miri64 commented Oct 4, 2016

Uh oh!

miri64 commented Oct 6, 2016

Uh oh!

miri64 commented Oct 6, 2016

Uh oh!

miri64 commented Oct 12, 2016

Uh oh!

haukepetersen commented Oct 25, 2016

Uh oh!

miri64 commented Oct 25, 2016

Uh oh!

miri64 commented Oct 25, 2016

Uh oh!

miri64 commented Oct 25, 2016

Uh oh!

cgundogan commented Oct 26, 2016

Uh oh!

cgundogan commented Oct 26, 2016

Uh oh!

miri64 commented Oct 26, 2016

Uh oh!

miri64 commented Oct 26, 2016

Uh oh!

miri64 commented Oct 26, 2016

Uh oh!

haukepetersen commented Oct 27, 2016

Uh oh!

Uh oh!

benpicco Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

miri64 commented Aug 26, 2016 •

edited by cgundogan

Loading

benpicco Nov 6, 2022 •

edited

Loading