[PATCH] 7006496: Use modern Windows API to retrieve OS DNS servers

[Chris Hegarty]

> On 27 Dec 2019, at 08:44, Anuraag Agrawal <anuraaga at gmail.com> wrote:
> 
> ...
> On Tue, Dec 24, 2019 at 11:57 PM Chris Hegarty <chris.hegarty at oracle.com <mailto:chris.hegarty at oracle.com>> wrote:
> 
> ...
> I think that this mainly looks good. A few small comments:
> 
> 1) If getAdapters returns an error ( ret != ERROR_SUCCESS ), then there will be a JNI pending exception, right? so loadDNSConfig0 should not unconditionally throw OOME.
> 
> Thanks for noticing this. Previously, any failure to malloc would return STS_ERROR and loadDNSConfig0 would throw OOME, while any other type of error (e.g., getAdaptersInfo returning an error) is ignored. In the current patch, failure to malloc sets OOME, otherwise a generic Error, and then the code tries to throw OOME which would probably cause a crash due to throwing two exceptions in a row without clearing (it's hard to repro the error case so not entirely sure).
> 
> To restore the old behavior, I would have to check the result of getAdapters and only throw if the exception is OOME, and ignore exceptions of type Error which are generated here
> 
> https://github.com/openjdk/jdk/blob/master/src/java.base/windows/native/libnet/NetworkInterface_winXP.c#L114 <https://github.com/openjdk/jdk/blob/master/src/java.base/windows/native/libnet/NetworkInterface_winXP.c#L114> 
> 
> Alternatively if leaving the exception of getAdapters as is and just remove the current JNU_ThrowOutOfMemoryError in loadDNSConfig0, we will throw Error when a random failure of GetAdapterAddresses happens. This is probably an incompatible change and not acceptable but just want to confirm.
> 
> Also it would be possible to skip getAdapters exceptions that are not OOME in C or in Java. It would be great to hear what the recommended pattern would be.

One approach would be to only throw if there is no pending exception, e.g.

    211     } else if (!(*env)->ExceptionOccurred(env)) {
    212         JNU_ThrowOutOfMemoryError(env, "native memory allocation failed");
    213     }

But looking at this again, I think that the OOM is not needed at all with the new code, since loadDNSConfig is not itself doing a malloc ( any more ) - and this catch-all style is great anyway.

>  ...
> 
> 3) I wonder if we should bump MAX_STR_LEN, while here?
> 
> Seems like a good idea - I don't know what a good value for this is but noticed another location uses 1K so have raised it to that in my local copy.


That seems fine to me.

-Chris.


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/net-dev/attachments/20200102/1b131e49/attachment.htm>