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
