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

[Xueming Shen]

On 4/8/14 10:29 AM, Seán Coffey 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 ?
>
My take is that the performance is not a concern here, the only real 
problem is the SEGV.
Given "num" is not a volatile and is not updated under synchronized 
block,  check "num == 0"
is not going to make ZFIS work for mult-thread usage. It also makes me 
nervous to check it
inside the synchronized block as a global "flag". I'm also concerned 
that the change to check
the rem == 0 after the check of "len" may also change the behavior of 
someone's code in
certain circumstance...

-Sherman

> 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.
>>
>