RFR: 8284910: Buffer clean in PasswordCallback [v2]

[Roger Riggs]

On Wed, 20 Apr 2022 19:59:16 GMT, Sean Mullan <mullan 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. 
>> 
>>> 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.
>> 
>> 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.
>
>> > 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.

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

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