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