On 08/04/2014 19:12, Pavel Rappo 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.
The ZipFile API itself is not thread safe and the example given would be buggy application code. However - we should cover this case. As per Sherman's last mail then, I'll make the change and check for zentry == 0 instead of rem variable. zentry is read and set under synchronization. just need to run it through JPRT.

One other note on the ZipFileInputStream.read(..) method :
            if (len <= 0) {
                return 0;
            }
It looks like the above check is not necessary since a negative len check is make in the native ZIP_READ function which this ends up calling (and has same behaviour / return value)

Regards,
Sean.

-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