RFR : 8038491: Improve synchronization in ZipFile.read()

[Chris Hegarty]

On 8 Apr 2014, at 18:57, Xueming Shen <xueming.shen at 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.
>>> 
>> 
>