On Sat, 8 Jan 2022 19:00:52 GMT, Mark Sheppard <[email protected]> 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
