Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8193682: Infinite loop in ZipOutputStream.close() #5522

Closed
wants to merge 14 commits into from

Conversation

raviniitw2012
Copy link

@raviniitw2012 raviniitw2012 commented Sep 15, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issues

  • JDK-8193682: Infinite loop in ZipOutputStream.close()
  • JDK-8276305: Infinite loop in ZipOutputStream.close() (CSR) ⚠️ Issue is not open.

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522
$ git checkout pull/5522

Update a local copy of the PR:
$ git checkout pull/5522
$ git pull https://git.openjdk.java.net/jdk pull/5522/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5522

View PR using the GUI difftool:
$ git pr show -t 5522

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5522.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 15, 2021

👋 Welcome back rreddy! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Sep 15, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 15, 2021

@raviniitw2012 The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs label Sep 15, 2021
@raviniitw2012
Copy link
Author

@raviniitw2012 raviniitw2012 commented Sep 15, 2021

Reviewers: @LanceAndersen @coffeys

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 15, 2021

closed = true;
out.close();
closed = true;
throw ioe;
Copy link
Contributor

@AlanBateman AlanBateman Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried using try-finally instead?

Copy link
Author

@raviniitw2012 raviniitw2012 Sep 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Alan , Sorry for the delayed response , I was out of office for 3 weeks. I haven't tried with try-finally , I'm reworking on the fix as , if we directly use zip.finish() , we are facing the same issue. I will update here once the fix is ready.Thanks.

@raviniitw2012
Copy link
Author

@raviniitw2012 raviniitw2012 commented Oct 11, 2021

I have updated the review with the new fix.

Instead of throwing an exception in close() method , Now when we get an exception during write inside deflate() , we will close the stream and throw the exception.

Updated the test case in TestNG format.

Mach5 : https://mach5.us.oracle.com/mdash/jobs/rreddy-jdk-20211011-1331-25193031/

@@ -249,7 +249,14 @@ public void close() throws IOException {
protected void deflate() throws IOException {
int len = def.deflate(buf, 0, buf.length);
if (len > 0) {
out.write(buf, 0, len);
try {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this use try with resources:
try (out) { ...

Copy link
Contributor

@coffeys coffeys Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the output stream is only closed if an exception is raised though ?

Copy link
Author

@raviniitw2012 raviniitw2012 Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are closing the stream only when an exception occurs during a write operation

out.write(buf, 0, len);
} catch (Exception e) {
def.end();
out.close();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out.close not needed with try with resources.

Copy link
Contributor

@AlanBateman AlanBateman Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@coffeys coffeys Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

out.write(buf, 0, len);
try {
out.write(buf, 0, len);
} catch (Exception e) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be a finally block?

private static byte[] b = new byte[FINISH_NUM];
private static Random rand = new Random();

@Test
Copy link
Contributor

@coffeys coffeys Oct 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can condense the test code to aid maintenance - please consider using DataProvider

Copy link
Contributor

@AlanBateman AlanBateman left a comment

Setting to "Request changes" until the spec impact is understood.

@raviniitw2012
Copy link
Author

@raviniitw2012 raviniitw2012 commented Oct 26, 2021

Hi ,

I have made the changes in fix , I think changing existing behavior in deflate() will need a spec change.
With this fix , I'm closing deflater on exception scenarios in finish method of GZipOutputStream and close method of DeflaterOutputStream. Even though the document for finsih/close methods does not clearly specifies if the deflater will be closed or not , the current behaviour of these methods does close the deflater whenever finish/close are called.

Thanks,
Ravi

} catch(IOException e) {
if (usesDefaultDeflater)
def.end();
throw e;
Copy link
Contributor

@AlanBateman AlanBateman Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting is a bit wonky but I think this version is the best so far. It does mean that the stream state is undefined when close throws but this has always been the case.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

I think overall this looks good. Thank you for continuing to work on this.

I do believe it would be worth adding a CSR just to document the behavior realizing that it was always left undefined in the past

Please also add a comment to each test describing its intent


public class GZipLoopTest {
private static final int FINISH_NUM = 512;
private static OutputStream outStream = new OutputStream() {
Copy link
Contributor

@LanceAndersen LanceAndersen Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment describing the intent of outstream and for FINISH_NUM. You might also consider a different name vs FINISH_NUM. Perhaps the comment will clarify this

zip.close();
} catch (IOException e) {
//expected
}
Copy link
Contributor

@LanceAndersen LanceAndersen Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the above if an IOException is expected, should this be tested via assertThrows()?


@Test
public void testGZipClose() throws IOException {
rand.nextBytes(b);
Copy link
Contributor

@LanceAndersen LanceAndersen Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could possibly consider using a BeforeTest or BeforeMethod if you choose to reduce redundancy. No biggie otherwise.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

I think we are closer.

The formatting is better. Thank you for changing the name for the constant.

Please add comments describing the intent of the test, DataProvider, and BeforeTest method to make it clear to future maintainers when they look back on the tests.

We should still create a CSR to highlight the change.

@openjdk openjdk bot removed the rfr label Nov 2, 2021
@openjdk openjdk bot added the rfr label Nov 2, 2021
@raviniitw2012
Copy link
Author

@raviniitw2012 raviniitw2012 commented Nov 2, 2021

Hello All .

Thanks for reviewing these changes.
While testing ZipOutputStream:closeEntry(), I have found the same infinite loop issue is reproducible. Since closeEntry() does internally close the deflater , even though the documentation does not specify it, I think it should be fine to give the similar fix in closeEntry() of ZipOutputStream as well. So there are total of three places where we should close deflater before throwing an exception.
1.GZipOutputStream:finish()
2.DeflaterOutputStream:close()
3.ZipOutputStream:closeEntry()

I have created a CSR explaining the changes: https://bugs.openjdk.java.net/browse/JDK-8276305

@raviniitw2012
Copy link
Author

@raviniitw2012 raviniitw2012 commented Nov 9, 2021

/csr needed

@openjdk openjdk bot added the csr label Nov 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 9, 2021

@raviniitw2012 this pull request will not be integrated until the CSR request JDK-8276305 for issue JDK-8193682 has been approved.

Copy link
Contributor

@LanceAndersen LanceAndersen left a comment

Hi Ravi,

The current revision looks good to me.

Best
Lance

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Nov 18, 2021

Hi Ravi,

The current revision looks good to me.

Best Lance

I agree, commit d911297 looks much better. There will be follow-up clarification needed to javadoc but they can be down with another JBS issue/PR.

…se the deflater incase of IOException not in ZipException case scenario
@raviniitw2012
Copy link
Author

@raviniitw2012 raviniitw2012 commented Nov 18, 2021

Hi All,

In the latest commit, I have resolved an issue in the case of ZipException in ZipOutputStream.closeEntry().
Now close the deflater in ZipOutputStream.closeEntry() only in case of an IOException and not with ZipException.

Thanks,
Ravi

@openjdk openjdk bot removed the csr label Nov 29, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 29, 2021

@raviniitw2012 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8193682: Infinite loop in ZipOutputStream.close()

Reviewed-by: lancea, coffeys

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 217 new commits pushed to the master branch:

  • abaa073: 8277946: NMT: Deprecate and remove VM.native_memory shutdown jcmd command option
  • 37ff7f3: 8277866: gc/epsilon/TestMemoryMXBeans.java failed with wrong initial heap size
  • 8d7958e: 8277981: String Deduplication table is never cleaned up due to bad dead_factor_for_cleanup
  • bc6dce1: 8277736: G1: Allow forced evacuation failure of first N regions in collection set
  • 0c29ee5: 8274319: Replace usages of Collections.sort with List.sort call in jdk.jfr
  • f505396: 8277459: Add jwebserver tool
  • 84aa0a1: 8278047: Few javax/imageio test regressed after JDK-8262297 fix
  • fde0b95: 8277861: Terminally deprecate Thread.stop
  • 70d5dff: 8275326: C2: assert(no_dead_loop) failed: dead loop detected
  • 349328c: 8277777: [Vector API] assert(r->is_XMMRegister()) failed: must be in x86_32.ad
  • ... and 207 more: https://git.openjdk.java.net/jdk/compare/7fc344dc96008f277dacf5518b28447f3a598cde...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AlanBateman, @LanceAndersen, @coffeys) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Nov 29, 2021
Copy link
Contributor

@coffeys coffeys left a comment

could you move the test up one directory to java/util/zip ? I don't think it's particular to GZIP any longer. Also perhaps a rename to something like CloseDeflaterTest etc.

coffeys
coffeys approved these changes Dec 1, 2021
@raviniitw2012
Copy link
Author

@raviniitw2012 raviniitw2012 commented Dec 1, 2021

/integrate

@openjdk openjdk bot added the sponsor label Dec 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2021

@raviniitw2012
Your change (at version 6541de4) is now ready to be sponsored by a Committer.

@coffeys
Copy link
Contributor

@coffeys coffeys commented Dec 1, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2021

Going to push as commit 1e9ed54.
Since your change was applied there have been 217 commits pushed to the master branch:

  • abaa073: 8277946: NMT: Deprecate and remove VM.native_memory shutdown jcmd command option
  • 37ff7f3: 8277866: gc/epsilon/TestMemoryMXBeans.java failed with wrong initial heap size
  • 8d7958e: 8277981: String Deduplication table is never cleaned up due to bad dead_factor_for_cleanup
  • bc6dce1: 8277736: G1: Allow forced evacuation failure of first N regions in collection set
  • 0c29ee5: 8274319: Replace usages of Collections.sort with List.sort call in jdk.jfr
  • f505396: 8277459: Add jwebserver tool
  • 84aa0a1: 8278047: Few javax/imageio test regressed after JDK-8262297 fix
  • fde0b95: 8277861: Terminally deprecate Thread.stop
  • 70d5dff: 8275326: C2: assert(no_dead_loop) failed: dead loop detected
  • 349328c: 8277777: [Vector API] assert(r->is_XMMRegister()) failed: must be in x86_32.ad
  • ... and 207 more: https://git.openjdk.java.net/jdk/compare/7fc344dc96008f277dacf5518b28447f3a598cde...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 1, 2021
@openjdk openjdk bot added integrated and removed ready rfr sponsor labels Dec 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Dec 1, 2021

@coffeys @raviniitw2012 Pushed as commit 1e9ed54.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs integrated
5 participants