On 8 Apr 2014, at 18:57, Xueming Shen <xueming.s...@oracle.com> wrote:
> > 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… To make this safe and simple, why not just move the close inside the synchronized block to ensure no concurrent access before close completes ( if needed ). There is very little computation overhead added to the synchronized block, but guarantees serial access to close. synchronized (ZipFile.this) { ensureOpenOrZipException(); len = ZipFile.read(ZipFile.this.jzfile, jzentry, pos, b, off, len); if (len > 0) { pos += len; rem -= len; } if (rem == 0) { close(); } } -Chris. > > -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. >>> >> >