8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary #269

LanceAndersen · 2020-09-20T18:03:02Z

Hi all,

Please review the fix for JDK-8252739 which addresses an issue introduced by https://bugs.openjdk.java.net/browse/JDK-8225189, where Deflater.c ignored the offset specified by Deflater.setDictionary.

Mach5 jdk-tier1, jdk-tier2, jdk-tier3 runs cleanly as well as the java/util/zip and java/util/jar JCK tests.

Progress

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

Issue

JDK-8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary

Reviewers

Uwe Schindler (@uschindler - Author) ⚠️ Review applies to aac03e3
Alan Bateman (@AlanBateman - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/269/head:pull/269
$ git checkout pull/269

… offset for the dictionary

bridgekeeper · 2020-09-20T18:04:23Z

👋 Welcome back lancea! 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 · 2020-09-20T18:05:51Z

@LanceAndersen 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 (add|remove) "label" command.

mlbridge · 2020-09-20T18:14:57Z

Webrevs

uschindler

The tests with byte buffers (direct and heap) are not using offsets (arrayOffset=0). The direct buffer test uses just a series of 0-bytes, so incorrect offsets won't change result. There should be real data copied into the direct buffer.

uschindler · 2020-09-20T18:25:38Z

test/jdk/java/util/zip/DeflaterDictionaryTests.java

+        byte[] input = SRC_DATA.getBytes(UTF_8);
+        byte[] output = new byte[RESULT_SIZE];
+        // Compress the bytes
+        ByteBuffer dictDef = ByteBuffer.wrap(DICTIONARY.getBytes(UTF_8));


This should use a slice of the byte buffer, so array offset is not 0. This can be done with offset and size on the wrap() call.

uschindler · 2020-09-20T18:26:52Z

test/jdk/java/util/zip/DeflaterDictionaryTests.java

+        byte[] input = SRC_DATA.getBytes(UTF_8);
+        byte[] output = new byte[RESULT_SIZE];
+        // Compress the bytes
+        ByteBuffer dictDef = ByteBuffer.allocateDirect(DICTIONARY.length());


This creates an empty dictionary with only 0 bytes. Maybe copy some real data into the buffer. Also a slice should be used.

mlbridge · 2020-09-20T18:32:58Z

Mailing list message from Uwe Schindler on core-libs-dev:

Hi,

I left some comments on the PR.

The tests with ByteBuffers are not testing the bug correctly.

Uwe

Am September 20, 2020 6:14:59 PM UTC schrieb Lance Andersen <lancea at openjdk.java.net>:

LanceAndersen · 2020-09-20T20:38:59Z

Minor updates have been made to the tests

uschindler · 2020-09-20T20:42:30Z

Ok much better for the heap buffers. Many thanks.

The direct buffers have now contents, but I think it should copy the whole heap array into the byte buffer. After that set position and limit and then call slice(). After that you have a slice of the original direct buffer with offset.

LanceAndersen · 2020-09-20T22:19:25Z

I have made additional updates to testByteBufferDirect to incorporate your suggestions

uschindler

Looks fine to me.

AlanBateman

Update to Deflater.c looks good.

DeflaterDictionaryTests looks like is a shaping up to be a good test for setDictionary. Are there other assertions that should be checked, e.g. setDictionary(ByteBuffer) is specified to consume all bytes and it would be good to check that the position is set to the limit. Also can the 2-arg setDictionary be tests, also corner cases such no bytes remaining or invoked with null.

The naming of the tests is a bit inconsistent, e.g. ByteArrayTest vs. testByteBufferDirect. In the naming then I would use "Heap" instead of "Wrap", as in testHeapByteBuffer.

In passing: The tests can use try-finally to ensure that Deflater::end is invoked even when the test fails. String repeat(int) could be used to avoid duplicating "Welcome to Us Open;". Missing space in several "if(...)" usages.

LanceAndersen · 2020-09-21T19:06:54Z

Update to Deflater.c looks good.

DeflaterDictionaryTests looks like is a shaping up to be a good test for setDictionary. Are there other assertions that should be checked, e.g. setDictionary(ByteBuffer) is specified to consume all bytes and it would be good to check that the position is set to the limit. Also can the 2-arg setDictionary be tests, also corner cases such no bytes remaining or invoked with null.

I have logged JDK-8253444 to add more coverage for setDictionary. I would prefer to put this change back to address the offset not being used and improve the coverage afterwards.

The naming of the tests is a bit inconsistent, e.g. ByteArrayTest vs. testByteBufferDirect. In the naming then I would use "Heap" instead of "Wrap", as in testHeapByteBuffer.

I updated the test name(s).

In passing: The tests can use try-finally to ensure that Deflater::end is invoked even when the test fails. String repeat(int) could be used to avoid duplicating "Welcome to Us Open;". Missing space in several "if(...)" usages.

Addressed the issues above.

I have pushed the updated changes for review to the PR.

AlanBateman

Given that the offset handling is buggy then I think the test should at least cover the cases where the offset is 0 or out of bounds. No problem separating out further test coverage for the other setDictionary methods into a separate issue.

LanceAndersen · 2020-09-22T20:54:41Z

Given that the offset handling is buggy then I think the test should at least cover the cases where the offset is 0 or out of bounds. No problem separating out further test coverage for the other setDictionary methods into a separate issue.

I updated the test to include an offset test for 0 and to add a test for out of range offsets. I have updated the PR.

AlanBateman

Thanks for update to cover the offset 0 and out of bound cases.

openjdk · 2020-09-23T06:59:49Z

@LanceAndersen This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary

Reviewed-by: uschindler, alanb

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 190 commits pushed to the master branch:

5f1d612: 8253397: Ensure LogTag types are sorted
b8ea80a: 8253457: Remove unimplemented register stack functions
e4d0e5a: 8253516: ZGC: Remove card table functions
3fe5886: 8252696: Loop unswitching may cause out of bound array load to be executed
226faa5: 8253241: Update comment on java_suspend_self_with_safepoint_check()
bd67975: 8253349: Remove unimplemented SharedRuntime::native_method_throw_unsupported_operation_exception_entry
bddb822: 8253240: No javadoc for DecimalFormatSymbols.hashCode()
c68a31d: 8253499: Problem list runtime/cds/DeterministicDump.java
93a2018: 8252195: AWT Accessibility API nested classes rely on default constructors
426c904: 8253392: remove PhaseCCP_DCE declaration
... and 180 more: https://git.openjdk.java.net/jdk/compare/4b3a0b789ebf389a9589c9af2d3922fb91916d92...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 5f1d6120a531cedcd0cb33568e04f79c649d0788.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

LanceAndersen · 2020-09-23T14:20:12Z

/integrate

openjdk · 2020-09-23T14:22:03Z

@LanceAndersen Since your change was applied there have been 190 commits pushed to the master branch:

5f1d612: 8253397: Ensure LogTag types are sorted
b8ea80a: 8253457: Remove unimplemented register stack functions
e4d0e5a: 8253516: ZGC: Remove card table functions
3fe5886: 8252696: Loop unswitching may cause out of bound array load to be executed
226faa5: 8253241: Update comment on java_suspend_self_with_safepoint_check()
bd67975: 8253349: Remove unimplemented SharedRuntime::native_method_throw_unsupported_operation_exception_entry
bddb822: 8253240: No javadoc for DecimalFormatSymbols.hashCode()
c68a31d: 8253499: Problem list runtime/cds/DeterministicDump.java
93a2018: 8252195: AWT Accessibility API nested classes rely on default constructors
426c904: 8253392: remove PhaseCCP_DCE declaration
... and 180 more: https://git.openjdk.java.net/jdk/compare/4b3a0b789ebf389a9589c9af2d3922fb91916d92...master

Your commit was automatically rebased without conflicts.

Pushed as commit 812b39f.

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

Deflater.setDictionary(byte[], int off, int len) ignores the starting…

7f3fd5d

… offset for the dictionary

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 20, 2020

LanceAndersen changed the title ~~RFR 8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary~~ 8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary Sep 20, 2020

openjdk bot added the rfr Pull request is ready for review label Sep 20, 2020

uschindler suggested changes Sep 20, 2020

View reviewed changes

Additional updates to DeflaterDictionaryTests.java

eaf0be8

More updates to testByteBufferDirect in DeflaterDictionaryTests.java

aac03e3

uschindler approved these changes Sep 20, 2020

View reviewed changes

AlanBateman reviewed Sep 21, 2020

View reviewed changes

More updates to testByteBufferDirect in DeflaterDictionaryTests.java

f34a670

AlanBateman reviewed Sep 22, 2020

View reviewed changes

Add additional offset test cases in DeflaterDictionaryTests.java

a37d9d2

AlanBateman approved these changes Sep 23, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 23, 2020

openjdk bot closed this Sep 23, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary #269

8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary #269

LanceAndersen commented Sep 20, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 20, 2020

openjdk bot commented Sep 20, 2020

mlbridge bot commented Sep 20, 2020 •

edited

Loading

uschindler left a comment

uschindler Sep 20, 2020

uschindler Sep 20, 2020

mlbridge bot commented Sep 20, 2020

LanceAndersen commented Sep 20, 2020

uschindler commented Sep 20, 2020 •

edited by bridgekeeper bot

Loading

LanceAndersen commented Sep 20, 2020

uschindler left a comment

AlanBateman left a comment

LanceAndersen commented Sep 21, 2020 •

edited

Loading

AlanBateman left a comment

LanceAndersen commented Sep 22, 2020

AlanBateman left a comment

openjdk bot commented Sep 23, 2020 •

edited

Loading

LanceAndersen commented Sep 23, 2020

openjdk bot commented Sep 23, 2020

8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary #269

8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary #269

Conversation

LanceAndersen commented Sep 20, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 20, 2020

openjdk bot commented Sep 20, 2020

mlbridge bot commented Sep 20, 2020 • edited Loading

Webrevs

uschindler left a comment

Choose a reason for hiding this comment

uschindler Sep 20, 2020

Choose a reason for hiding this comment

uschindler Sep 20, 2020

Choose a reason for hiding this comment

mlbridge bot commented Sep 20, 2020

LanceAndersen commented Sep 20, 2020

uschindler commented Sep 20, 2020 • edited by bridgekeeper bot Loading

LanceAndersen commented Sep 20, 2020

uschindler left a comment

Choose a reason for hiding this comment

AlanBateman left a comment

Choose a reason for hiding this comment

LanceAndersen commented Sep 21, 2020 • edited Loading

AlanBateman left a comment

Choose a reason for hiding this comment

LanceAndersen commented Sep 22, 2020

AlanBateman left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 23, 2020 • edited Loading

LanceAndersen commented Sep 23, 2020

openjdk bot commented Sep 23, 2020

LanceAndersen commented Sep 20, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 20, 2020 •

edited

Loading

uschindler commented Sep 20, 2020 •

edited by bridgekeeper bot

Loading

LanceAndersen commented Sep 21, 2020 •

edited

Loading

openjdk bot commented Sep 23, 2020 •

edited

Loading