> On 27 Dec 2019, at 08:44, Anuraag Agrawal <[email protected]> wrote:
>
> ...
> On Tue, Dec 24, 2019 at 11:57 PM Chris Hegarty <[email protected]
> <mailto:[email protected]>> 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.
