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

nsapi - Fix leftover bytes from suffix during ipv6 parsing #2953

Merged
merged 3 commits into from
Oct 25, 2016
Merged

nsapi - Fix leftover bytes from suffix during ipv6 parsing #2953

merged 3 commits into from
Oct 25, 2016

Conversation

geky
Copy link
Contributor

@geky geky commented Oct 6, 2016 •
edited
Loading

Thanks to @EduardPon for hunting this down:

In the function ipv6_from_address(), address values of IPv6 address after a :: is found are moved to the end of the short[] array. But the source memory of the moved part should also be cleared with zeros before the address values in front of the :: are copied. In the current implementation this may lead to an invalid address conversion.

fixes #2931

@geky geky mentioned this pull request Oct 6, 2016
Closed
@geky
Copy link
Contributor Author

geky commented Oct 6, 2016

/morph test-nightly

c1728p9
c1728p9 reviewed Oct 6, 2016
View reviewed changes
features/netsocket/SocketAddress.cpp
@@ -122,6 +122,7 @@ static void ipv6_from_address(uint8_t *bytes, const char *addr)
// Move suffix to end
memmove(&shorts[NSAPI_IPv6_BYTES/2-suffix], &shorts[0],
suffix*sizeof(uint16_t));
memset(&shorts[0], 0, suffix*sizeof(uint16_t));
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to assert size of suffix to catch overflows before they occur.

Copy link

@EduardPon EduardPon Oct 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This memset solution not more expensive (slower you are writing 2x bytes instead of 1x words (uint16_t) ). The variable shortsis already a uint16_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@c1728p9, the value of suffix is bounded by this loop:
https://github.com/geky/mbed/blob/16c4da03e62fc2367b33be8bb9358c28804fffb3/features/netsocket/SocketAddress.cpp#L88

I don't think there is much value to adding an assert at this granularity, there are many other points of failure in this function.

@EduardPon, memset and friends can be optimized quite heavily by the compiler, usually emitting word-aligned assignments. The loop may be quicker, but without a test to check which is faster I just went with the higher-level operation.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I mention adding an assert is because you have no array bounds check for shorts and you get the size to zero from a separate function. Clobbered stacks can be tricky to track down, since a crash typically occurs on the function return.

@mbed-bot
Copy link

mbed-bot commented Oct 6, 2016

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1070

Test failed!

@kjbracey
Copy link
Contributor

kjbracey commented Oct 7, 2016

Fix is good.

You'll find another set of 12 useful tests in the IPv6<->string conversion functions over in Nanomesh libservice:

https://github.com/ARMmbed/nanostack-libservice/

Covers all the tricky cases from RFC 5952, but most of the trickiness is actually in binary->string conversion.

Of course, we could just use the existing implementations from there. Seems a bit silly debugging or improving these implementations when we already have a fully-compliant pair. Believe the only problem is its locked behind FEATURE_COMMON_PAL (and the name "nanostack" in the path). Enable that feature for netsocket?

mikaleppanen
mikaleppanen reviewed Oct 7, 2016
View reviewed changes
features/netsocket/SocketAddress.cpp
@@ -122,6 +122,7 @@ static void ipv6_from_address(uint8_t *bytes, const char *addr)
// Move suffix to end
memmove(&shorts[NSAPI_IPv6_BYTES/2-suffix], &shorts[0],
suffix*sizeof(uint16_t));
memset(&shorts[0], 0, suffix*sizeof(uint16_t));
Copy link

Choose a reason for hiding this comment

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

I think that length of the memset should be (NSAPI_IPv6_BYTES/2-suffix)*sizeof(uint16_t)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The remaining space has already been zeroed out by the previous memset:
https://github.com/geky/mbed/blob/16c4da03e62fc2367b33be8bb9358c28804fffb3/features/netsocket/SocketAddress.cpp#L110

Either way would work

Copy link

Choose a reason for hiding this comment

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

If suffix is long memsetting suffix size can overwrite the beginning of memmoved suffix e.g.
if address is 2001::db8:1111:2222:3333:4444

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, this should be updated

@geky
Copy link
Contributor Author

geky commented Oct 7, 2016

It would be a good idea to adopt the nanomesh ipv6 parsing and tests. I'm hesitant to create another dependency on FEATURE_COMMON_PAL, but we could replicate the implementation to reuse code.

In the meantime this patch provides a fix for the current implementation.

@geky geky added needs: CI and removed needs: work labels Oct 7, 2016
@bridadan
Copy link
Contributor

bridadan commented Oct 7, 2016

/morph test-nightly

@mbed-bot
Copy link

mbed-bot commented Oct 7, 2016

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 1077

Test failed!

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 10, 2016

Result: FAILURE

I reviewed some of the failures, I would highlight these:

NRF51_DK-GCC_ARM.tests-netsocket-ip_parsing.Simple IPv4 address
KL46Z-GCC_ARM.tests-netsocket-ip_parsing.Simple IPv4 address

@geky
Copy link
Contributor Author

geky commented Oct 19, 2016

/morph test-nightly

@geky
Copy link
Contributor Author

geky commented Oct 19, 2016

/morph test-nightly

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 917

Test failed!

@mbed-bot
Copy link

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test-nightly

Output

mbed Build Number: 918

Test failed!

@geky
Copy link
Contributor Author

geky commented Oct 20, 2016

Looks like the results are good 👍

Failing tests are the lp_ticker for the NCS36510

@0xc0170 0xc0170 merged commit d7c02a1 into ARMmbed:master Oct 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EduardPon EduardPon EduardPon left review comments

@mikaleppanen mikaleppanen mikaleppanen left review comments

@c1728p9 c1728p9 c1728p9 left review comments

Assignees
No one assigned
Labels
release-version: 5.2.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SocketAddress.cpp ipv6_from_address does not clear byte who have been moved. FIX included
9 participants
@geky @mbed-bot @kjbracey @bridadan @0xc0170 @c1728p9 @mikaleppanen @EduardPon @sg-