On 8 Apr 2014, at 19:41, Xueming Shen <xueming.s...@oracle.com> wrote:

> On 4/8/14 11:33 AM, Chris Hegarty wrote:
>> On 8 Apr 2014, at 19:12, Pavel Rappo <pavel.ra...@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.

Ok, sounds reasonable.

-Chris.

> 
> -Sherman
> 
>> 
>>> -Pavel
>>> 
>>> On 8 Apr 2014, at 18:29, Seán Coffey <sean.cof...@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.
> 

Reply via email to