Fixes IPv6 multicast join issue

[sathishm6]

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]

@0xc0170 @AnttiKauppila - Please review. Thanks.

[ciarmcom]

@sathishm6, thank you for your changes.
@AnttiKauppila @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

[kjbracey]

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.

[kjbracey]

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")

[sathishm6]

@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.

[kjbracey]

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.

[sathishm6]

@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.

[sathishm6]

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

[kjbracey]

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]

CI started

[mbed-ci]

Test run: SUCCESS

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