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.

-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