RFR: 8260401: StackOverflowError on open WindowsPreferences

[Jaikiran Pai]

On Mon, 1 Feb 2021 19:21:23 GMT, Brian Burkhalter <bpb at openjdk.org> wrote:

>> Can I please get a review for this change which proposes to fix the issue reported in https://bugs.openjdk.java.net/browse/JDK-8260401?
>> 
>> As noted in that issue, when the constructor of `java.util.prefs.WindowsPreferences` runs into an error while dealing with the Windows registry, it logs a warning message. The message construction calls `rootNativeHandle()` (on the same instance of `WindowsPreferences` that's being constructed) which then ultimately ends up calling the constructor of `WindowsPreferences` which then again runs into the Windows registry error and attempts to log a message and this whole cycle repeats indefinitely leading to that `StackOverFlowError`. 
>> 
>> Based on my limited knowledge of the code in that constructor and analyzing that code a bit, it's my understanding (and also the input provided by the reporter of the bug) that the log message should actually be using the `rootNativeHandle` parameter that is passed to this constructor instead of invoking the `rootNativeHandle()` method. The commit in this PR does this change to use the passed `rootNativeHandle` in the log message.
>> 
>> Furthermore, there's another constructor in this class which does a similar thing and potentially can run into the same error as this one. I've changed that constructor too, to avoid the call to `rootNativeHandle()` method in that log message. Reading the log message that's being generated there, it's my understanding that this change shouldn't cause any inaccuracy in what's being logged and in fact, I think it's now more precise about which handle returned the error code.
>> 
>> Finally, this log message creation, in both the constructors, also calls `windowsAbsolutePath()` and `byteArrayToString()` methods. The `byteArrayToString()` is a static method and a call to it doesn't lead back to the constructor again. The `windowsAbsolutePath()` is a instance method and although it does use a instance variable `absolutePath`, that instance variable is already initialized appropriately by the time this `windowsAbsolutePath()` gets called in the log message. Also, the `windowsAbsolutePath()` call doesn't lead back to the constructor of the `WindowsPreferences` so it doesn't pose the same issue as a call to `rootNativeHandle()`.
>> 
>> Given the nature of this issue, I haven't added any jtreg test for this change.
>
> The code change looks all right. It would be better if there were a test, but apparently this might depend on the user who runs the test not having registry access rights.

Hello Brian,

Thank you for the review.

> It would be better if there were a test, but apparently this might depend on the user who runs the test not having registry access rights.

That's correct. Looking at the code, it looks to me that this will require very specific setup of the Windows system to be able to trigger the error.

> The code change looks all right.

Should I go ahead and integrate this?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2326