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

udp: fix wrong usage of pkt pointer #3918

Closed
wants to merge 1 commit into from
Closed

udp: fix wrong usage of pkt pointer #3918

wants to merge 1 commit into from

Conversation

cgundogan
Copy link
Member

This PR fixes the hard fault described in #3815. @authmillenon can you verify?

@cgundogan cgundogan added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking labels Sep 21, 2015
@miri64
Copy link
Member

miri64 commented Sep 21, 2015

I don't see how this is different from the current situation :-/. Can you explain to me? If I see it correctly, due to https://github.com/RIOT-OS/RIOT/pull/3918/files#diff-a743a0dcf90d0eb32c837f1a047918c1L106 the current code is equivalent to your code.

cgundogan
cgundogan reviewed Sep 21, 2015
View reviewed changes
sys/net/gnrc/transport_layer/udp/gnrc_udp.c
@@ -130,9 +130,9 @@ static void _receive(gnrc_pktsnip_t *pkt)
port = (uint32_t)byteorder_ntohs(hdr->dst_port);

/* send payload to receivers */
if (!gnrc_netapi_dispatch_receive(GNRC_NETTYPE_UDP, port, pkt)) {
if (!gnrc_netapi_dispatch_receive(GNRC_NETTYPE_UDP, port, udp)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe it's just this line that did the hard fault.. need to test if the other lines are necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems to be just this line that breaks the current state. But I don't think this is really a solution here... I think you intented to send the payload to the listener and not the udp snip itself. This is what I changed here now. pktdump confirms, it begins printing with the udp snip instead of the payload. So the actual problem may be somewhere else .. maybe in pktdump?

Copy link
Member

Choose a reason for hiding this comment

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

What no! The listener is supposed to get the whole packet, not just the payload. And you are not sending the payload, you are sending the UDP header... at least when the meaning of the variables is still the same as it was.

Copy link
Member

Choose a reason for hiding this comment

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

(confirmed by my packet dump

1442859872.171887;m3-9;> udp send fe80::3432:4833:46d5:9d12 1337 hey
1442859872.172833;m3-9;PKTDUMP: data received:
1442859872.173814;m3-9;~~ SNIP  0 - size:   8 byte, type: NETTYPE_UDP (4)
1442859872.174802;m3-9;   src-port:  1337  dst-port:  1337
1442859872.174906;m3-9;   length: 11  cksum: 0x45664
1442859872.177047;m3-9;~~ SNIP  1 - size:  40 byte, type: NETTYPE_IPV6 (2)
1442859872.177160;m3-9;traffic class: 0x00 (ECN: 0x0, DSCP: 0x00)
1442859872.177850;m3-9;flow label: 0x00000
1442859872.178827;m3-9;length: 11  next header: 17  hop limit: 64
1442859872.179832;m3-9;source address: fe80::3432:4833:46d5:9d12
1442859872.180833;m3-9;destination address: fe80::3432:4833:46d5:9d12
1442859872.181832;m3-9;~~ PKT    -  2 snips, total size:  48 byte
1442859872.182815;m3-9;Success: send 40 byte to fe80::3432:4833:46d5:9d12:1337

where is the payload? ;-))

Copy link
Member Author

Choose a reason for hiding this comment

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

Well in current master it is the payload (pktsnip of type undef). I think this this is what is actually right. And I believe the solution that I presented here is wrong :/

@miri64
Copy link
Member

miri64 commented Sep 21, 2015

I can however confirm that the node no longer crashes (the packet buffer still contains (technically legal) some stuff however):

Program received signal SIGINT, Interrupt.
idle_thread (arg=<optimized out>) at /home/martine/Repositories/RIOT-OS/RIOT/core/kernel_init.c:73
73          if (lpm_prevent_sleep) {
(gdb) print *_first_unused 
$5 = {next = 0x20001cc0 <_pktbuf+136>, size = 104}
(gdb) print *_first_unused->next
$6 = {next = 0x0, size = 6008}
(gdb) print 136-104
$7 = 32
(gdb) x/32xb _pktbuf+104
0x20001ca0 <_pktbuf+104>:   0x02    0x1c    0x00    0x20    0x08    0x00    0x00    0x00
0x20001ca8 <_pktbuf+112>:   0x08    0x00    0x00    0x00    0x01    0x00    0x00    0x00
0x20001cb0 <_pktbuf+120>:   0x4c    0x1c    0x00    0x20    0xf0    0x1c    0x00    0x20
0x20001cb8 <_pktbuf+128>:   0x03    0x00    0x00    0x00    0x00    0x1c    0x00    0x20

Someone an idea what this stuff might be?

@miri64
Copy link
Member

miri64 commented Sep 21, 2015

(This is not related to this PR)

@miri64
Copy link
Member

miri64 commented Sep 21, 2015

ACK and go when travis goes 🔔

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 21, 2015
@cgundogan
Copy link
Member Author

please see my inline comment

@miri64
Copy link
Member

miri64 commented Sep 21, 2015

(ACK revoked if that wasn't clear)

@miri64
Copy link
Member

miri64 commented Sep 22, 2015

Can we close this? #3925 seems to fix it.

@cgundogan
Copy link
Member Author

closed in favour of #3925

@cgundogan cgundogan closed this Sep 22, 2015
@miri64 miri64 added the State: invalid State: The issue / PR is not valid label Sep 22, 2015
@cgundogan cgundogan deleted the pr/udp/self branch October 1, 2015 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@miri64 miri64

Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR State: invalid State: The issue / PR is not valid Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cgundogan @miri64