RFR: 8284910: Buffer clean in PasswordCallback [v2]

[Stuart Marks]

On Wed, 20 Apr 2022 21:09:54 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>>> > Won't there be a small performance hit (perhaps negligible) for code that already calls clearPassword?
>>> 
>>> The impact should be minimal. If clearPassword() has been called, the cleanup (Cleanerable.clean()) won't happen again in the finalization or setPassword cleanup.
>> 
>> I don't think that is true, but maybe I am missing something. From looking at the code, it appears a `clearPassword` followed by a `setPassword` would call `Arrays.fill` twice on the same password. I think this can be fixed by setting the cleaner to null in the `clearPassword` method after the password has been cleared. I think this would address my concern and we may not need a spec. update (though I want to think it thru one more time).
>> 
>>> > A specification clarification would provide clarity to applications that they do not have to call clearPassword in between calls to setPassword.
>>> 
>>> As far as I know from the JDK code, it might be not common to call clearPassword in between calls to setPassword. I would like to have applications calling clearPassword() methods as before, if it is not missed, to speed up the collection rather than rely on finalization.
>> 
>> Yes, I agree calling `clearPassword` should be recommended as a best practice and callers should not solely rely on cleaners.
>> 
>>> The relationship among setPassword, getPassword and clearPassword() is complicated. I fully agree that the spec should be clarified. I would like to have the clarify update in another PR, and have this one focus on cleanup if an application forget to call clearPassword properly.
>> 
>> See above.
>
> Calling `Cleanable.clean()` calls the `Runnable` action at-most-once.
> Each `Cleanable` inserted in a list when it is created and is removed when `clear()` or `clean()` is invoked.
> The action is called only if it is still in the list. So there is no extra overhead.
> 
> There is no harm in clearing the cleanable field; but it does not save much.
> 
> The code in `clearPassword` can be simplified and only test `cleanable != null`; it will be null unless there is an inputPassword to clean.

I'd recommend setting `cleanable` to null after it's been cleaned to make the state machine easier to reason about. The invariant would be: if `cleanable` is non-null, then we have something dirty that needs to be cleaned. If we don't clear it to null after cleaning, it potentially results in confusing states. For example, suppose the app calls `setPassword(nonNull)` and later calls `setPassword(null)`. The second call will set `inputPassword` to null but leave a stale reference in `cleanable`. This isn't necessarily harmful, but it's confusing.

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

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