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