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

gnrc_ipv6: fix SEGFAULT when multicasting with multiple interfaces #12512

Merged
merged 1 commit into from
Oct 21, 2019
Merged

gnrc_ipv6: fix SEGFAULT when multicasting with multiple interfaces #12512

merged 1 commit into from
Oct 21, 2019

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Oct 20, 2019 •
edited
Loading

Contribution description

When writing to the IPv6 header the implementation currently doesn't take the packet with the (potentially) duplicated header, but the packet with the original one, which leads to the packet sent and then released in gnrc_netif_ethernet.c first and then accessed again in further iterations of the "writing to the IPv6 header" loop, which causes access to an invalid pointer, causing a crash.

Testing procedure

When repeating the "Steps to reproduce" in #11980, the node should not crash and you get a lot of (duplicate) replies. When using two interfaces with one being a 6LoWPAN interface, the node should not crash, pinging might however not work due to the bug fixed in #10499.

Issues/PRs references

Fixes #11980

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 20, 2019
@miri64 miri64 added this to the Release 2019.10 milestone Oct 20, 2019
@miri64 miri64 requested review from benpicco and kb2ma October 20, 2019 10:50
@miri64 miri64 mentioned this pull request Oct 20, 2019
Closed
miri64
miri64 commented Oct 20, 2019
View reviewed changes
sys/net/gnrc/network_layer/ipv6/gnrc_ipv6.c
if (prep_hdr) {
DEBUG("ipv6: prepare IPv6 header for sending\n");
/* need to get second write access (duplication) to fill IPv6
* header interface-local */
gnrc_pktsnip_t *tmp = gnrc_pktbuf_start_write(pkt);
send_pkt = gnrc_pktbuf_start_write(pkt);
Copy link
Member Author

Choose a reason for hiding this comment

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

I also renamed that variable because tmp was a bit unintuitive ;-)

@miri64 miri64 changed the title gnrc_ipv6.c: fix SEGFAULT when multicasting with multiple interfaces gnrc_ipv6: fix SEGFAULT when multicasting with multiple interfaces Oct 20, 2019
@miri64
gnrc_ipv6: fix SEGFAULT when multicasting with multiple interfaces
ce14ee1 
When writing to the IPv6 header the implementation currently doesn't
take the packet with the (potentially) duplicated header, but the
packet with the original one, which leads to the packet sent and then
released in `gnrc_netif_ethernet.c` first and then accessed again in
further iterations of the "writing to the IPv6 header" loop, which
causes access to an invalid pointer, causing a crash.

Fixes RIOT-OS#11980
@miri64 miri64 force-pushed the gnrc_ipv6/fix/i11980 branch from 25a6782 to ce14ee1 Compare October 20, 2019 12:25
@miri64 miri64 mentioned this pull request Oct 20, 2019
Closed
80 tasks
@miri64 miri64 added the Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch label Oct 20, 2019
benpicco
benpicco approved these changes Oct 20, 2019
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Works as expected :)

Only tested it on native now, I can try it out on a device with two 802.15.4 interfaces tomorrow, but that shouldn't stall merging - if multi-interface multicast brings more bugs to the surface, that would be a separate issue.

This was referenced Oct 20, 2019
Merged
Merged
@miri64 miri64 merged commit 8b2f697 into RIOT-OS:master Oct 21, 2019
@miri64
Copy link
Member Author

miri64 commented Oct 21, 2019

Thanks for reviewing and testing!

@miri64 miri64 deleted the gnrc_ipv6/fix/i11980 branch October 21, 2019 07:44
@miri64 miri64 mentioned this pull request Oct 21, 2019
Merged
@miri64
Copy link
Member Author

miri64 commented Oct 21, 2019

Backport provided in #12521

@miri64 miri64 mentioned this pull request Oct 21, 2019
Merged
@benpicco
Copy link
Contributor

And for completeness' sake:

2019-10-21 11:47:51,579 #  ping6 ff02::1
2019-10-21 11:47:51,593 # 12 bytes from fe80::5806:59c9:2c36:7d79: icmp_seq=0 ttl=64 rssi=-17 dBm time=6.612 ms
2019-10-21 11:47:51,601 # 12 bytes from fe80::5807:59c9:2c36:7d79: icmp_seq=0 ttl=64 rssi=-33 dBm time=14.306 ms (DUP!)
2019-10-21 11:47:52,599 # 12 bytes from fe80::5806:59c9:2c36:7d79: icmp_seq=1 ttl=64 rssi=-17 dBm time=6.620 ms
2019-10-21 11:47:52,607 # 12 bytes from fe80::5807:59c9:2c36:7d79: icmp_seq=1 ttl=64 rssi=-33 dBm time=14.314 ms (DUP!)
2019-10-21 11:47:53,636 # 12 bytes from fe80::5806:59c9:2c36:7d79: icmp_seq=2 ttl=64 rssi=-17 dBm time=6.631 ms
2019-10-21 11:47:53,637 # 
2019-10-21 11:47:53,639 # --- ff02::1 PING statistics ---
2019-10-21 11:47:53,645 # 3 packets transmitted, 3 packets received, 2 duplicates, 0% packet loss
2019-10-21 11:47:53,650 # round-trip min/avg/max = 6.612/9.696/14.314 ms

@miri64 miri64 restored the gnrc_ipv6/fix/i11980 branch October 31, 2019 18:50
@miri64 miri64 deleted the gnrc_ipv6/fix/i11980 branch October 31, 2019 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@kb2ma kb2ma Awaiting requested review from kb2ma

Assignees
No one assigned
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 Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Milestone
Release 2019.10
Development

Successfully merging this pull request may close these issues.

Sending a packet to the Multicast Address with multiple interfaces causes Segmentation fault
2 participants
@miri64 @benpicco