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.

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

Fixes IPv6 multicast join issue #11667

Merged
merged 1 commit into from Oct 15, 2019

Conversation

@sathishm6
Copy link

sathishm6 commented Oct 10, 2019

Description

Fixes IPv6 multicast join issue

Problem Statement:
During multicast join sequence, InternetSocket::join_multicast_group() calls InternetSocket::modify_multicast_group(). modify_multicast_group() sets up the multicast group address (i.e., mreq.imr_multiaddr) to be joined and the interface address (i.e., mreq.imr_interface) to be used for the multicast join request. The interface address is initialized with the default value, which sets the version of interface address to NSAPI_UNSPEC. This results in LWIP::setsockopt() API to attempt IPv6 multicast join on the IPv4 interface address, hence IPv6 multicast join always fails with the protocol error.

Fix:
Initialize interface address version based on the multicast address version in LWIP::setsockopt(), before attempting multicast join operation.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@AnttiKauppila

Release Notes

@sathishm6

This comment has been minimized.

Copy link
Author

sathishm6 commented Oct 10, 2019

@0xc0170 @AnttiKauppila - Please review. Thanks.

@ciarmcom ciarmcom requested review from AnttiKauppila and ARMmbed/mbed-os-maintainers Oct 10, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Oct 10, 2019

@@ -566,6 +566,14 @@ nsapi_error_t LWIP::setsockopt(nsapi_socket_t handle, int level, int optname, co
return NSAPI_ERROR_PARAMETER;
}
} else {
/* Initialize the interface address type based on the multicast address version */

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 14, 2019

Contributor

The line below is supposed to be doing this, but it seems it has an error. Rather than adding code, I think you can just change it to

ip_addr_set_any(IP_IS_V6(&multi_addr), &if_addr);

Kind of surprised this didn't bring up a compiler warning about reading an uninitialised varriable (if_addr.type). I guess compilers don't track structure members for those warnings so much. Valgrind memcheck would have spotted it.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 14, 2019

Contributor

I agree with adjusting the comments, but this has left it a bit wonky, as line 563 already says what we're doing with the whole if/else.

Suggest removing that line, and having two separate comments in the if / else blocks - "Convert the interface address" / "Set interface address to "any", matching the group address type".

(All the other comments say "group address" rather than "multicast address", so that keeps it consistent - but you could change them all to "multicast address")

This comment has been minimized.

Copy link
@sathishm6

sathishm6 Oct 14, 2019

Author

@kjbracey-arm - Done the changes in the latest patch. That's a good catch! Thanks for it.

Ideally the compiler should catch such issues, as long as the right build options are enabled. For example, in case of GCC build, this will be shown as a warning if -Wall build option is included in the compile command. Adding -Werror along with it can help to stop the build on such issues. It would be good to check if -Wall is included in the build flow of mbed, so that, such issues can be identified and fixed early by leveraging the compiler. Thanks.

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 14, 2019

Contributor

For example, in case of GCC build, this will be shown as a warning if -Wall build option is included in the compile command.

Oh, this is enabled, but compilers will only generally spot int x; if (x) { ... }, not MyStruct x; if (x.a) { ... }.

This is because the warning is usually a side-effect of an optimisation pass that kicks in for register-sized objects. Unless the structure is being split, that optimisation and hence warning doesn't occur.

This comment has been minimized.

Copy link
@sathishm6

sathishm6 Oct 14, 2019

Author

@kjbracey-arm - Corrected the comments as well. Prefer to leave the comments with 'group address' since it anyway refers to the multicast group address. Thanks.

This comment has been minimized.

Copy link
@sathishm6

sathishm6 Oct 14, 2019

Author

Oh, this is enabled, but compilers will only generally spot int x; if (x) { ... }, not MyStruct x; if (x.a) { ... }.

Okay, may be you are right. But I'm not completely sure if optimization would mask such warnings. I have written simple C test application and it can catch such issues in the warning.

#include <stdio.h>

typedef struct one_try
{
    int a;
    int c;
} one;

int main(void)
{
    one test;
    int val = -1;

    if (test.a != 0)
    {
        val = 1;
    }
    else
    {
        val = 0;
    }
    printf("One.a = [%d]\n", val);
    return 0;
}

Build string:
$ gcc -Wall -Werror test.c
test.c: In function ‘main’:
test.c:16:13: error: ‘test.a’ is used uninitialized in this function [-Werror=uninitialized]
if (test.a != 0)
~~~~^~
cc1: all warnings being treated as errors

This comment has been minimized.

Copy link
@kjbracey-arm

kjbracey-arm Oct 14, 2019

Contributor

I think that is the case where it has managed to split it, because it can see you only ever access the members, never the structure. It's trivially easy to make the warning go away. Just reference the structure, eg:

one *p = &test;

or even

(void) &test;

Or change if (test.a != 0) to if (*&test.a != 0) .

The last one is a bit sad, because that sort of thing is something that tends to happen in macros or inline functions - spurious address-taking.

The compiler gets cautious whenever it sees a structure's address taken. Ideally it would be able to spot "pointless" address taking.

@0xc0170 0xc0170 added needs: work and removed needs: review labels Oct 14, 2019
@sathishm6 sathishm6 force-pushed the sathishm6:pr/ipv6-multi-cast-join-fix branch from 37a2fab to 92e56cd Oct 14, 2019
Sathish Kumar Mani
Problem Statement:
During multicast join sequence, InternetSocket::join_multicast_group() calls InternetSocket::modify_multicast_group(). modify_multicast_group() sets up the multicast group address (i.e., mreq.imr_multiaddr) to be joined and the interface address (i.e., mreq.imr_interface) to be used for the multicast join request. The interface address is initialized with the default value, which sets the version of interface address to NSAPI_UNSPEC. This results in LWIP::setsockopt() API to attempt IPv6 multicast join on the IPv4 interface address, hence IPv6 multicast join always fails with the protocol error.

Fix:
Initialize interface address version based on the multicast address version in LWIP::setsockopt(), before attempting multicast join operation.
@sathishm6 sathishm6 force-pushed the sathishm6:pr/ipv6-multi-cast-join-fix branch from 92e56cd to 5b791a4 Oct 14, 2019
@0xc0170 0xc0170 added needs: CI and removed needs: work labels Oct 14, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Oct 14, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Oct 15, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 7522f9c into ARMmbed:master Oct 15, 2019
25 checks passed
25 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8679 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8464B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.