On Sat, 8 Jan 2022 19:00:52 GMT, Mark Sheppard <mshep...@openjdk.org> wrote:
>> Daniel Jelinski has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address problems reported by clang-tidy > > src/java.base/windows/native/libnet/NetworkInterface_winXP.c line 271: > >> 269: curr = *netifPP; >> 270: ret = lookupIPAddrTable(env, &tableP); >> 271: if (ret == -1) { > > on the face of it, this change looks reasonable as one might think "let the > processing continue for IPv6", however it introduces some problems in > subsequent processing. Most notably, it assigns a NULL value to tableP , > which is then fed to the function enumAddresses_win_ipaddrtable and this > performs a sanity check for tableP == NULL and returns 0. Also > enumAddresses_win_ipaddrtable takes the inout parameter netaddr *netaddrP; > which is a local variable (stack variable) which hasn't been properly > initialized and only has a valid value assigned for a successful return from > enumAddresses_win_ipaddrtable. Thus, this inout varaiable has indeterminates > values (i.e. what ever is on the stack) which are then assigned in the else > block (for the return 0 case) > else{ > curr->addrs = netaddrP; > curr->naddrs += ret; > curr = curr->next; > } > and so subsequent indeterminate processing and potental crash is significant > as the netif data structures spurious pointer assignments > > I think the current coded return policy is good, but should probably include > *netifPP = NULL as nothing is being returned. > > The function getAllInterfacesAndAddresses also inconsistently handles the out > value of *netifPP as it is not always assigned a value in > enumAddresses_win_ipaddrtable. As such the addition of call to free_netif on > this pointer may lead to indeterminate behavious -- Thus the assignment and > update of netifPP needs to be addressed I modified all functions to return sensible pointer values (possibly null) in all success (nonnegative) return cases. Returned pointers may contain nonsense only in failure cases. In particular, `enumAddresses_win_ipaddrtable` was fixed to set `*netaddrPP = NULL;` in 784d5fb332aee4215c02751636c848ffbb0da2b9. > src/java.base/windows/native/libnet/NetworkInterface_winXP.c line 282: > >> 280: } >> 281: while (curr != NULL) { >> 282: netaddr *netaddrP; > > netaddr *netaddrP = NULL; just in case enumAddresses_win_ipaddrtable does > not update it for an error return netaddrP is always updated when `enumAddresses_win_ipaddrtable` returns a nonnegative value > src/java.base/windows/native/libnet/NetworkInterface_winXP.c line 285: > >> 283: ret = enumAddresses_win_ipaddrtable(env, curr, &netaddrP, >> tableP); >> 284: if (ret < 0) { >> 285: free_netif(*netifPP); > > seems like the right thing to do, but *netifPP may not be properly assigned a > valid pointer `netifPP` is always assigned by `enumInterfaces` in nonnegative return cases. If `enumInterfaces` returns a negative value, this function (`getAllInterfacesAndAddresses`) aborts much earlier. ------------- PR: https://git.openjdk.java.net/jdk/pull/6090