Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:46:33 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
>>
>>> 254: } catch (Exception e) {
>>> 255: def.end();
>>> 256: out.close();
>>
>> out.close not needed with try with resources.
>
> This changes deflate to close the compressor and the output stream when there
> is an I/O exception. I expect this will need a spec change or a
> re-examination of the issue to see if there are alternatives.
the out.close() call could be removed I guess. Leave it for user code to handle
etc. Safer for spec also.
Main goal is to break the looping of the deflate call. The usesDefaultDeflater
boolean might be useful in helping determine if def.end() should be called or
not. If that boolean is false, then maybe we could just alter the input buffer
by setting it to a size 0 buffer (ZipUtils.defaultBuf) -- worth a look.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy wrote: >> Hi all, >> >> Please review this fix for Infinite loop in ZipOutputStream.close(). >> The main issue here is when ever there is an exception during close >> operations on GZip we are not setting the deflator to a finished state which >> is leading to an infinite loop when we try writing on the same GZip >> instance( since we use while(!def.finished()) inside the write operation). >> >> Thanks, >> Ravi > > Ravi Reddy has updated the pull request incrementally with one additional > commit since the last revision: > > 8193682 : Infinite loop in ZipOutputStream.close() Setting to "Request changes" until the spec impact is understood. - Changes requested by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:35:17 GMT, Sean Coffey wrote:
>> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
>>
>>> 250: int len = def.deflate(buf, 0, buf.length);
>>> 251: if (len > 0) {
>>> 252: try {
>>
>> Shouldn't this use try with resources:
>> try (out) { ...
>
> the output stream is only closed if an exception is raised though ?
Yes , we are closing the stream only when exception occurs during write
operation
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 15:00:11 GMT, Ravi Reddy wrote: >> the output stream is only closed if an exception is raised though ? > > Yes , we are closing the stream only when exception occurs during write > operation Yes, we are closing the stream only when an exception occurs during a write operation - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:11:15 GMT, jmehrens wrote:
>> Ravi Reddy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8193682 : Infinite loop in ZipOutputStream.close()
>
> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
>
>> 254: } catch (Exception e) {
>> 255: def.end();
>> 256: out.close();
>
> out.close not needed with try with resources.
This changes deflate to close the compressor and the output stream when there
is an I/O exception. I expect this will need a spec change or a re-examination
of the issue to see if there are alternatives.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:10:53 GMT, jmehrens wrote:
>> Ravi Reddy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8193682 : Infinite loop in ZipOutputStream.close()
>
> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
>
>> 250: int len = def.deflate(buf, 0, buf.length);
>> 251: if (len > 0) {
>> 252: try {
>
> Shouldn't this use try with resources:
> try (out) { ...
the output stream is only closed if an exception is raised though ?
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy wrote: >> Hi all, >> >> Please review this fix for Infinite loop in ZipOutputStream.close(). >> The main issue here is when ever there is an exception during close >> operations on GZip we are not setting the deflator to a finished state which >> is leading to an infinite loop when we try writing on the same GZip >> instance( since we use while(!def.finished()) inside the write operation). >> >> Thanks, >> Ravi > > Ravi Reddy has updated the pull request incrementally with one additional > commit since the last revision: > > 8193682 : Infinite loop in ZipOutputStream.close() test/jdk/java/util/zip/GZIP/GZipLoopTest.java line 55: > 53: private static Random rand = new Random(); > 54: > 55: @Test I think we can condense the test code to aid maintenance - please consider using DataProvider - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy wrote:
>> Hi all,
>>
>> Please review this fix for Infinite loop in ZipOutputStream.close().
>> The main issue here is when ever there is an exception during close
>> operations on GZip we are not setting the deflator to a finished state which
>> is leading to an infinite loop when we try writing on the same GZip
>> instance( since we use while(!def.finished()) inside the write operation).
>>
>> Thanks,
>> Ravi
>
> Ravi Reddy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8193682 : Infinite loop in ZipOutputStream.close()
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
> 250: int len = def.deflate(buf, 0, buf.length);
> 251: if (len > 0) {
> 252: try {
Shouldn't this use try with resources:
try (out) { ...
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 254:
> 252: try {
> 253: out.write(buf, 0, len);
> 254: } catch (Exception e) {
Shouldn't this be a finally block?
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
> 254: } catch (Exception e) {
> 255: def.end();
> 256: out.close();
out.close not needed with try with resources.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
> Hi all, > > Please review this fix for Infinite loop in ZipOutputStream.close(). > The main issue here is when ever there is an exception during close > operations on GZip we are not setting the deflator to a finished state which > is leading to an infinite loop when we try writing on the same GZip instance( > since we use while(!def.finished()) inside the write operation). > > Thanks, > Ravi Ravi Reddy has updated the pull request incrementally with one additional commit since the last revision: 8193682 : Infinite loop in ZipOutputStream.close() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/f6a678ed..d18eb3c1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=01-02 Stats: 46 lines in 1 file changed: 44 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
