Zombie FileHandler locks can exhaust all available log file locks.

[Daniel Fuchs]

Hi Jason,

Thanks for the feedback!

On 6/24/14 7:08 PM, Jason Mehrens wrote:
> Daniel,
>
>
> With regard to JDK-4420020, in the original webrev Jim was incorrectly using java.io.File.canWrite() but that webrev was replaced by the current version. The NIO.2 code performs the effective access checks correctly.

Thanks.

> With regard to JDK-6244047 my concern was that checking the permissions and preconditions up front is a small 'TOCTOU' race and redundant because the next step after that is to open the FileChannel. I would assume both CREATE or CREATE_NEW plus WRITE would perform the effective access checks on open.

OK.

> The use of delete on exit is a deal breaker because you can't undo a registration on close of the FileHandler which can trigger an OOME. See https://bugs.openjdk.java.net/browse/JDK-4872014. I've used custom proxy handlers that generate a file name based off of the LogRecord which results in generating lots of file names and opening and close the FileHandler on demand.

ah. hmmmm. I didn't think there would be that many FileHandlers...
Well - I guess that if we find a way to reuse the zombie
lock files then we don't really need the delete on exit.
Someone creating a FileHandler programmatically should be responsible
for closing it - so if an application creates FileHandlers without
closing them then it's a bug in the application.

> If the behavior is reverted to use CREATE instead of CREATE_NEW then we have to deal with JDK-8031438 because any user code can lock a file ahead of opening the FileHandler.

Yes - I discovered that while writing a test for my suggested fix ;-)
Catching OverlappingFileLockException in FileHandler.openFiles
and setting available to false when it's thrown fixes the issue.

So let's just remove the deleteOnExit & add the catch for 
OverlappingFileLockException to my patch and we should be
good.

On the other hand we could just use replace CREATE_NEW by
CREATE but I have some misgivings about the part that
sets available to true when tryLock throws an IOException.
I'm not sure we should be doing that if we haven't actually
created the lock file. So I think I'd prefer keeping the
two steps behavior - first try CREATE_NEW - then attempt
to use CREATE if CREATE_NEW failed...
I'm not sure CREATE will check the access rights for writing
in the directory if the file already exists - so checking
the directory for write access in that case is probably
the right thing to do.

Would you agree?

-- daniel

>
> Jason
>
>
>
> ----------------------------------------
>> Date: Mon, 23 Jun 2014 17:13:04 +0200
>> From: daniel.fuchs at oracle.com
>> To: Alan.Bateman at oracle.com; jason_mehrens at hotmail.com; core-libs-dev at openjdk.java.net
>> Subject: Re: Zombie FileHandler locks can exhaust all available log file locks.
>>
>> On 6/23/14 4:53 PM, Alan Bateman wrote:
>>> On 23/06/2014 10:48, Daniel Fuchs wrote:
>>>> :
>>>>
>>>> All in all - I feel our best options would be to revert to using
>>>> CREATE, or use CREATE_NEW + DELETE_ON_CLOSE, or not do anything
>>>> and live with the issue.
>>>> Hopefully some nio experts will chime in ;-)
>>>>
>>> The logging use of FileLock is a bit unusual but looking at it now then
>>> I don't see the reason why it needs to use CREATE_NEW.
>>
>> OK - thanks - in the discussion that ended up in the patch where
>> CREATE_NEW was introduced Jason (I think it was Jason) pointed at
>> https://bugs.openjdk.java.net/browse/JDK-4420020
>>
>> I'm guessing here that maybe it would be wrong to use an already
>> existing lock file if you don't have the rights to write to
>> its directory - and that using CREATE_NEW ensures that you have
>> all necessary access rights all along the path.
>>
>>> I don't think
>>> DELETE_ON_CLOSE is suitable here as that work is for transient work
>>> files which isn't the case here. Instead it could use deleteOnExit to
>>> have some chance of removing it on a graceful exit.
>>
>> Right - I suggested a patch in another mail where I just use that.
>>
>>>
>>> BTW: Do you know why locks is Map? Would a Set do?
>>
>> It certainly looks as if we could use a simple HashSet here.
>>
>> Thanks Alan!
>>
>> -- daniel
>>
>>>
>>> -Alan.
>>>
>>