RFR : 8038491: Improve synchronization in ZipFile.read()

[Xueming Shen]

On 4/8/14 11:33 AM, Chris Hegarty wrote:
> On 8 Apr 2014, at 19:12, Pavel Rappo <pavel.rappo at oracle.com> wrote:
>
>> Hi Sean, Sherman
>>
>> as far as the client is using ZipFileInputStream in a multithreaded fashion (de facto), don't we have to rethink synchronization for the machinery of ZipFileInputStream.read? As far as I understand it's not enough to put this particular read of 'rem' into the synchronized block, as there are
>> at least 2 more accesses to it (and only in the 'read' method). We're reading it in the synchronized block, fine, but all the corresponding reads/writes have to be done with respect to 'happens-before' relationship between them.
> Agreed. There doesn’t appear, or at least I cannot find, any statement in ZipFile about its thread-safety ( or lack there of ), but the implementation of ZipFile appears to be “somewhat” thread-safe. The InputStreams returned by getInputStream(ZipEntry) are not. Sean’s testcase clearly shows that a single InputStream is being shared across multiple threads. Unless we want to make these streams thread-safe then we probably should not make any changes here.
>

It is a reasonable use scenario to close a java.io.InputStream via a 
different thread and it
definitely should not lead to a SEGV. But java.io.InputStream itself is 
not "thread-safe". To
read from a java.io.InputStream via multiple thread is kinda of useless 
use scenario, which
normally means unpredictable reading result for those read operation.

-Sherman

>
>> -Pavel
>>
>> On 8 Apr 2014, at 18:29, Seán Coffey <sean.coffey at oracle.com> wrote:
>>
>>> Sherman,
>>>
>>> I see rem == 0 condition becoming true before the zentry variable is set to 0 (in close()) - in a multi threaded scenario like this one, we could have zero remaining bytes in zentry and yet have a zentry != 0 - your suggestion would prevent the SEGV (since we're in the sync block) but we would needlessly cycle into the ensureOpenOrZipException() and ZipFile.read code (line 713) if another thread tasked with the close() call was knocked off by the dispatcher during the various rem == 0 checks that we make : e.g
>>>
>>> 722             if (rem == 0) {
>>> 723                 close();
>>> 724             }
>>>
>>> Am I reading this correctly ?
>>>
>>> regards,
>>> Sean.
>>>
>>> On 08/04/2014 16:59, Xueming Shen wrote:
>>>> Hi Sean,
>>>>
>>>> It might be more explicit to check "if (zentry == 0)" here?
>>>>
>>>> -Sherman
>>>>
>>>> On 4/8/14 8:21 AM, Seán Coffey wrote:
>>>>> A recently reported bug shows a race condition is possible on the rem == 0 check in ZipFile.read(byte b[], int off, int len). A bad check can result in referencing a jzentry structure that might already be freed and hence result in a SEGV. The fix is trivial and involves moving the rem == 0 check into a synchronized block. The ZipFile API itself is not thread safe so having mutiple threads operate on the same ZipFileInputStream is something that should never be performed.
>>>>>
>>>>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8038491/webrev/
>>>>>
>>>>> regards,
>>>>> Sean.