Skip to content

gnrc_ipv6_nib: fix for 'holey' NIB#7926

Merged
miri64 merged 2 commits intoRIOT-OS:gnrc_netif2_integration/masterfrom
miri64:gnrc_ipv6_nib/fix/iter-over-holes
Nov 9, 2017
Merged

gnrc_ipv6_nib: fix for 'holey' NIB#7926
miri64 merged 2 commits intoRIOT-OS:gnrc_netif2_integration/masterfrom
miri64:gnrc_ipv6_nib/fix/iter-over-holes

Conversation

@miri64
Copy link
Copy Markdown
Member

@miri64 miri64 commented Nov 1, 2017

When there are holes in the NIB (e.g. when entries were removed) currently the NIB crashes the system due to a failed assertion (DEVELHELP needs to be activated to test this behavior).

This fixes this behavior by making the assertion a check that is always compiled in.

Test cases to show the bug before this fix are provided (again: DEVELHELP needs to be activated to show the crash).

@miri64 miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) GNRC Area: network Area: Networking labels Nov 1, 2017
@miri64 miri64 requested a review from cgundogan November 1, 2017 18:41
@miri64 miri64 mentioned this pull request Nov 1, 2017
Merged
3 tasks
@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 Nov 1, 2017
@miri64 miri64 force-pushed the gnrc_ipv6_nib/fix/iter-over-holes branch from 7682c2c to 51b3834 Compare November 1, 2017 18:50
@miri64 miri64 mentioned this pull request Nov 1, 2017
Merged
28 tasks
@bergzand
Copy link
Copy Markdown
Member

bergzand commented Nov 8, 2017

Is the iterator in sys/net/gnrc/network_layer/ipv6/nib/nib_nc.c not affected by this issue?

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 8, 2017

TL;DR: No.

Only the off-link types are affected by that because they have the next_hop pointer (the neighbor cache itself is of the type the next_hop), which was assumed to always be not NULL in the iteration (which isn't the case if e.g. an entry in-between was removed). For more information on the internal structure of the NIB see my (incomplete) model.

bergzand
bergzand requested changes Nov 9, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

A few small nittpicks, nothing too serious here.

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c Outdated
}

/**
* Creates three default routes and removes one
Copy link
Copy Markdown
Member

@bergzand bergzand Nov 9, 2017

Choose a reason for hiding this comment

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

Maybe clarify here that the first one is removed: "…and removes the first one."

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-ft.c Outdated
}

/**
* Creates three prefix based routes and removes the second one
Copy link
Copy Markdown
Member

@bergzand bergzand Nov 9, 2017

Choose a reason for hiding this comment

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

Missing period.

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-pl.c
* Expected result: there should be two prefix list entries returned, the first
* and the third one
*/
static void test_nib_pl_iter__empty_in_the_middle(void)
Copy link
Copy Markdown
Member

@bergzand bergzand Nov 9, 2017

Choose a reason for hiding this comment

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

Why test here only the removal of the middle entry and not also the removal of the first as with the routes?

Copy link
Copy Markdown
Member Author

@miri64 miri64 Nov 9, 2017

Choose a reason for hiding this comment

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

I only tested the removal of the first one with the routes because for default routes I have no control which one is removed (which I realize might be an issue iff people have multiple default routes [which on an embedded system I would highly discourage anyway], but this is not the topic of this PR)

Copy link
Copy Markdown
Member

@bergzand bergzand Nov 9, 2017

Choose a reason for hiding this comment

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

Ok

tests/unittests/tests-gnrc_ipv6_nib/tests-gnrc_ipv6_nib-pl.c Outdated
}

/**
* Creates three prefix list entries and removes the second one
Copy link
Copy Markdown
Member

@bergzand bergzand Nov 9, 2017

Choose a reason for hiding this comment

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

missing period.

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 9, 2017

Addressed comments

bergzand
bergzand approved these changes Nov 9, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@bergzand bergzand left a comment

Choose a reason for hiding this comment

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

Changes look good.

@miri64 miri64 force-pushed the gnrc_ipv6_nib/fix/iter-over-holes branch from b11f182 to dcac0a5 Compare November 9, 2017 13:36
@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 9, 2017

Squashed

@miri64
Copy link
Copy Markdown
Member Author

miri64 commented Nov 9, 2017

Oops rebased to wrong branch -.-

miri64 added 2 commits November 9, 2017 15:28
@miri64
tests: unittests: extend gnrc_ipv6_nib unittests for holey NIB
fa81932 
When there are holes in the NIB (e.g. when entries were removed)
currently the NIB crashes the system due to a failed assertion.
This commit tests this behavior (`DEVELHELP` needs to be activated to
test this behavior). A fix will follow.
@miri64
gnrc_ipv6_nib: fix for 'holey' NIB
72db5e4 
When there are holes in the NIB (e.g. when entries were removed)
currently the NIB crashes the system due to a failed assertion
(`DEVELHELP` needs to be activated to test this behavior).

This fixes this behavior by making the assertion a check that is always
compiled in.
@miri64 miri64 force-pushed the gnrc_ipv6_nib/fix/iter-over-holes branch from dcac0a5 to 72db5e4 Compare November 9, 2017 14:29
@miri64 miri64 merged commit b187c04 into RIOT-OS:gnrc_netif2_integration/master Nov 9, 2017
@miri64 miri64 deleted the gnrc_ipv6_nib/fix/iter-over-holes branch November 9, 2017 20:42
@aabadie aabadie added this to the Release 2018.01 milestone Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergzand bergzand bergzand approved these changes

@cgundogan cgundogan Awaiting requested review from cgundogan

Assignees

@cgundogan cgundogan

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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2018.01

Development

Successfully merging this pull request may close these issues.

4 participants

@miri64 @bergzand @cgundogan @aabadie