On 8 Apr 2014, at 18:57, Xueming Shen <[email protected]> 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.
>>>
>>
>
