RFR:JDK-8021372 NetworkInterface.getNetworkInterfaces() returns duplicate hardware address

[Mark Sheppard]

Thanks for the feedback.

yes, we can merge the conditional,
that was my comment about the getAdapter function and the if else statement
because of a spurious IPv4 comment.

the 267(old) and 281(new) is the core of the issue

old

262                     if (nif->index == index) {
  263                         /* found the interface entry
  264                          * set the index to the IPv6 index and add the
  265                          * IPv6 addresses
  266                          */
  267                         nif->index = ptr->Ipv6IfIndex;
  268                         c = getAddrsFromAdapter(ptr, &nif->addrs);
  269                         nif->naddrs += c;
  270                         break;
  271                     }

new

276                     if (nif->index == index) {
  277                         /* found the interface entry
  278                          * set the index to the IPv6 index and add the
  279                          * IPv6 addresses
  280                          */
  281                         nif->ipv6Index = ptr->Ipv6IfIndex;
  282                         c = getAddrsFromAdapter(ptr, &nif->addrs);
  283                         nif->naddrs += c;
  284                         break;
  285                     }

the nif->index is already set in the IPv4 case, hence if (nif->index == index) .
In the original code the assignment was overwriting this value with
Ipv6IfIndex, but this can be zero, if IPv6 is not available for that adapter, but IPv6 is enabled in the OS.
Hence, NetworkInterface.getHardwareAddress returns the physical address associated with index 0.
When, both IPv4 and IPv6 are configured for an interface then this discrepancy
is not seen.
As netif has an ipv6Index member, the Ipv6IfIndex assignment seemed appropriate.

This is then supplemented with an additional check against the Ipv6IfIndex member of IP_ADAPTER_ADDRESSES when checking
the index.

when both IPv4 and IPv6 are available on an interface, then the index should be the same.
As such, an OS can be enabled for both IPv4 and IPv6, while an interface may be configured for
IPv4 only, which coincidentally was how I managed to reproduce the issue!

regards
Mark

On 03/09/2013 11:27, Michael McMahon wrote:
> On 03/09/13 10:45, Dmitry Samersoff wrote:
>> Mark,
>>
>> 1. If I read the code correctly,
>>
>> ll. 168 - 177 is exactly the same as ll. 180 - 189
>>
>> Could you refactor the fix to avoid code duplication?
>>
>> Would something like
>>
>> if (
>>     (ptr->IfIndex != 0 && ptr->IfIndex == index) ||
>>     (ptr->Ipv6IfIndex != 0 && ptr->Ipv6IfIndex == index)
>> ) {
>> ...
>> }
>>
>> work?
>
> I think this makes sense.
>
> Also, I'm not sure I understand the purpose of the change
> at line 267 (old) 281 (new). What the old code seemed to be doing
> was interpreting an IPv6 index as actually an IPv4 one.
> Was that not correct? This code is rather brittle unfortunately
> and not the easiest to understand.
>
> Michael
>
>
>
>>
>> 2. It seems to me passed index couldn't be 0 so null check is redundant,
>> but I don't mind to keep it.
>>    It might make sense to explicitly check passed index for 0 above 
>> loop.
>>
>> -Dmitry
>>
>>
>>
>> On 2013-09-03 13:23, Mark Sheppard wrote:
>>> Hi
>>> please oblige and review the fix below to address the issue in 
>>> JDK-8021372:
>>> NetworkInterface.getNetworkInterfaces() returns duplicate hardware 
>>> address
>>>
>>> http://cr.openjdk.java.net/~msheppar/8021372/webrev/
>>>
>>> the handling of the Ipv6IfIndex was suspect when setting the
>>> interface index and when retrieving the mac address using the index.
>>>
>>> in the getAdpaters function, the conditaionalstatement could be rolled
>>> into one.
>>> But, as there was a spurious IPv4 comment, I structured it as an if
>>> else. statement.
>>>
>>> It should be noted that IfIndex and the Ipv6IfIndex can be zero, which
>>> implies that the
>>> IPv4 or IPv6 interface is not available. This may be at odds with the
>>> setting of a default index to 0 in the Java NetworkInterface 
>>> abstraction,
>>> where a defaultIndex of zero is used.
>>>
>>> JPRTs have been run and show no side effects.
>>>
>>> regards
>>> Mark
>>
>