8249783: Simplify DerValue and DerInputStream #232

wangweij · 2020-09-17T23:00:54Z

This code change rewrites DerValue into a mostly immutable class and simplifies DerInputStream as a wrapper for a series of DerValues objects. DerInputBuffer is removed.

All existing methods of DerValue and DerInputStream should still work with the exact same behavior, except for a few places where bugs are fixed. For example, Indefinite length must be used with a constructed tag.

Except for the ObjectIdentifier class where DerInputBuffer is directly referenced, no other code is touched.

Progress

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

Issue

JDK-8249783: Simplify DerValue and DerInputStream

Reviewers

Valerie Peng (@valeriepeng - Reviewer) ⚠️ Review applies to ac77172

Download

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

bridgekeeper · 2020-09-17T23:02:35Z

👋 Welcome back weijun! 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-17T23:03:58Z

@wangweij The following label will be automatically applied to this pull request: security.

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-17T23:07:58Z

Webrevs

…ructed value.

valeriepeng · 2020-09-18T23:56:01Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

-     */
-    public DerInputStream(byte[] data) throws IOException {
-        init(data, 0, data.length, true);
+    public DerInputStream(byte[] data, int start, int length, boolean allowBER) {


Add javadoc for the new impl? Do we need to validate things like data != null, start and length have valid values? Or assuming that since all its callers are internal, no need to check.

I don't intend to add any check because callers are internal. I can add a comment.

Adding a comment would be good.

valeriepeng · 2020-09-18T23:59:31Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

-     */
-    public DerInputStream(byte[] data, int offset, int len,
-        boolean allowBER) throws IOException {
-        init(data, offset, len, allowBER);


Add javadoc for the new impl?

valeriepeng · 2020-09-19T00:11:52Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

-    DerInputStream(DerInputBuffer buf) {
-        buffer = buf;
-        buffer.mark(Integer.MAX_VALUE);
+    public byte[] toByteArray() {


Add javadoc? Returns the remaining unread bytes? The name may lead people to expect the bytes returned are the one passing to the constructor.

Well. This method is sometimes used to read remaining bytes and sometimes used to read all (suppose none has been read yet). I will clarify.

valeriepeng · 2020-09-19T00:36:23Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

+    // The static part
+    final byte[] data;
+    final int start;
+    final int end;


Well, end is really "start + length", the data range is from start to (end -1) (inclusive).

Will add some comments.

valeriepeng · 2020-09-19T00:40:01Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

-    public byte[] toByteArray() {
-        return buffer.toByteArray();
+    public DerValue getDerValue() throws IOException {
+        DerValue result = new DerValue(


Check (this.end - this.pos > 0) before calling DerValue()?

new DerValue() would fail in this case.

valeriepeng · 2020-09-25T19:18:43Z

src/java.base/share/classes/sun/security/util/DerValue.java

+     * @param generalized true if Generalized Time is to be read, false
+     * if UTC Time is to be read.
+     */
+    private Date getTime(int len, boolean generalized) throws IOException {


You have the Internal suffix on other private helper routines. Maybe add it here as well?

valeriepeng · 2020-09-25T19:31:03Z

src/java.base/share/classes/sun/security/util/DerValue.java

+            throw new IOException("DER UTC Time length error");
+
+        data.pos = data.end; // Compatibility. Reach end.
+        return getTime(end - start, false);


Given that getTime() have access to all the fields, it seems redundant to have to specify (end-start) when calling it.

valeriepeng · 2020-09-25T19:53:31Z

src/java.base/share/classes/sun/security/util/DerValue.java

-        } catch (IOException e) {
-            throw new IllegalArgumentException("misformatted DER value");
-        }
+        return String.format("DerValue(%02x, %s, %d, 5d)",


5d should be %d?

valeriepeng · 2020-09-25T21:23:20Z

src/java.base/share/classes/sun/security/util/DerValue.java

+     * @param newTag the new tag
+     * @return a new DerValue
+     */
+    public DerValue withTag(byte newTag) {


Seems like a somewhat dangerous method. The value may not match the new tag? The caller is expected to know what it's doing?

I intend to use it to turn back IMPLICIT element to its original tag, otherwise getXyz() calls would fail because they are checking the tag. In the old design, checking the tag (in DerValue) and reading the value (in DerInputBuffer) are separated but now they are in a single method (in DerValue).

The caller pattern should always look like withTag(INTEGER).getInteger().

Hmm, I see, maybe add more comments then.

valeriepeng · 2020-09-25T21:24:58Z

src/java.base/share/classes/sun/security/util/DerValue.java

+    }
+
+    DerValue[] subs(byte expectedTag, int startLen) throws IOException {
+        if (expectedTag != 0 && expectedTag != tag) {


so if expectedTag == 0, then we don't check for tag match? Looks strange. If this is intentional, we should add comment to clarify why.

For IMPLICIT. Will comment.

wangweij

Thanks a lot for your detailed code review. I'll push a new commit early next week.

wangweij · 2020-09-25T22:45:01Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

+    // The static part
+    final byte[] data;
+    final int start;
+    final int end;


Will add some comments.

wangweij · 2020-09-25T22:47:42Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

-     */
-    public DerInputStream(byte[] data) throws IOException {
-        init(data, 0, data.length, true);
+    public DerInputStream(byte[] data, int start, int length, boolean allowBER) {


I don't intend to add any check because callers are internal. I can add a comment.

wangweij · 2020-09-25T22:52:59Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

-     */
-    public DerInputStream(byte[] data, int offset, int len,
-        boolean allowBER) throws IOException {
-        init(data, offset, len, allowBER);


wangweij · 2020-09-25T22:54:17Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

-    DerInputStream(DerInputBuffer buf) {
-        buffer = buf;
-        buffer.mark(Integer.MAX_VALUE);
+    public byte[] toByteArray() {


Well. This method is sometimes used to read remaining bytes and sometimes used to read all (suppose none has been read yet). I will clarify.

wangweij · 2020-09-25T22:58:26Z

src/java.base/share/classes/sun/security/util/DerInputStream.java

-    public byte[] toByteArray() {
-        return buffer.toByteArray();
+    public DerValue getDerValue() throws IOException {
+        DerValue result = new DerValue(


new DerValue() would fail in this case.

wangweij · 2020-09-26T00:49:24Z

src/java.base/share/classes/sun/security/util/DerValue.java

+            throw new IOException("DER UTC Time length error");
+
+        data.pos = data.end; // Compatibility. Reach end.
+        return getTime(end - start, false);


wangweij · 2020-09-26T00:49:52Z

src/java.base/share/classes/sun/security/util/DerValue.java

-        } catch (IOException e) {
-            throw new IllegalArgumentException("misformatted DER value");
-        }
+        return String.format("DerValue(%02x, %s, %d, 5d)",


wangweij · 2020-09-26T00:52:38Z

src/java.base/share/classes/sun/security/util/DerValue.java

+     * @param newTag the new tag
+     * @return a new DerValue
+     */
+    public DerValue withTag(byte newTag) {


I intend to use it to turn back IMPLICIT element to its original tag, otherwise getXyz() calls would fail because they are checking the tag. In the old design, checking the tag (in DerValue) and reading the value (in DerInputBuffer) are separated but now they are in a single method (in DerValue).

The caller pattern should always look like withTag(INTEGER).getInteger().

wangweij · 2020-09-26T00:53:03Z

src/java.base/share/classes/sun/security/util/DerValue.java

+    }
+
+    DerValue[] subs(byte expectedTag, int startLen) throws IOException {
+        if (expectedTag != 0 && expectedTag != tag) {


For IMPLICIT. Will comment.

wangweij · 2020-09-26T00:56:35Z

test/jdk/sun/security/util/DerValue/DeepOctets.java

+        // DerInputStream::getOctetString does not
+        Utils.runAndCheckException(
+                () -> new DerInputStream(input).getOctetString(),
+                IOException.class);


The existing impl already differs, but DerValue::getOctetString can only read one level of constructed OCTET STRING. My second commit can read arbitrary levels and hence this this regression test. There are two levels of 0x24 here.

wangweij · 2020-09-29T15:24:57Z

New change pushed. I haven't really moved the "deprecated" codes together in DerValue. I just put the data-related and tag-related methods together and add more comments.

Besides following your suggestions in the comment, this time I made some fine tuned changes for indefinite length parsing, esp, for the uncertainty about remaining data after conversion (see the updated Indefinite.java test). In the future, we can enhance DerIndefLenConverter to be more precise and efficient.

valeriepeng · 2020-09-30T22:55:21Z

src/java.base/share/classes/sun/security/util/DerInputStream.java


+        int value, tmp;
        String mdName = "DerInputStream.getLength(): ";
        tmp = lenByte;
        if ((tmp & 0x080) == 0x00) { // short form, 1 byte datum
            value = tmp;
        } else {                     // long form or indefinite


Can't be indefinite here given the new check (line 233-235 above?) Move the indefinite comment upward?

valeriepeng

Updated webrev looks fine. Only a minor nit regarding a comment remaining.

openjdk · 2020-10-01T02:25:28Z

@wangweij 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 more details.

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

8249783: Simplify DerValue and DerInputStream

Reviewed-by: valeriep

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 110 new commits pushed to the master branch:

05a764f: 8253883: Problem list jdk/test/lib/hexdump/ASN1Formatter on Windows
55c282b: 8253878: clean up nsk/share/jvmti/ArgumentHandler
776acfd: 8253880: clean up sun/hotspot/tools/ctw/Utils class
79d70f6: 8253869: sun/hotspot/whitebox/CPUInfoTest.java fails after JDK-8239090
ca0e014: 8252003: remove usage of PropertyResolvingWrapper in vmTestbase/nsk/jvmti
092c227: 8252523: Add ASN.1 Formatter to work with test utility HexPrinter
06d8cf6: 8253812: Cleanup AbstractMemberWriter
424d7d6: 8252881: [JVMCI] ResolvedJavaType.resolveMethod fails in fastdebug when invoked with a constructor
2a406f3: 8138732: Rename @HotSpotIntrinsicCandidate to @IntrinsicCandidate and move it to the jdk.internal.vm.annotation package
4b16f8a: 8253872: ArgumentHandler must use the same delimiters as in jvmti_tools.cpp
... and 100 more: https://git.openjdk.java.net/jdk/compare/b8ea80af33ecc9d4ccfa3c3dc366e1a88c3607b4...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.

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

wangweij · 2020-10-01T03:40:25Z

Added a new commit. Mostly comment or style change. Several always-false if (lenByte < 0) check removed. result.toArray(new DerValue[0]) used because new knowledge shows it's faster than result.toArray(new DerValue[result.size()]). getSet(int,boolean implicit) added check for implicit to be semantically correct (although it's always called with implicit == true.

wangweij · 2020-10-01T03:51:34Z

/test tier1, tier2

openjdk · 2020-10-01T03:52:01Z

@wangweij the test group tier2 does not exist

wangweij · 2020-10-01T18:54:42Z

/integrate

openjdk · 2020-10-01T18:55:54Z

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

9230c2a: 8253747: tools/jpackage/share/AppImagePackageTest.java fails with InstalledPackageSize: 0
cfd41c0: 8232840: java/math/BigInteger/largeMemory/SymmetricRangeTests.java fails due to "OutOfMemoryError: Requested array size exceeds VM limit"
8fda5b8: 8253904: Revert Tokenizer improvements JDK-8224225
60ec2a5: 8253824: Revert JDK-8253089 since VS warning C4307 has been disabled
90c131f: 8224225: Tokenizer improvements
9670425: 8253822: Remove unused exception_address_is_unpack_entry
8440279: 8180514: TestPrintMdo.java test fails with -XX:-TieredCompilation
44e6820: 8253650: Cleanup: remove alignment_hint parameter from os::reserve_memory
ed62b01: 6646602: Spelling error in javadoc for javax.swing.tree.TreeModel
87276bc: 6690021: typos in TransferHandler Javadoc
... and 118 more: https://git.openjdk.java.net/jdk/compare/b8ea80af33ecc9d4ccfa3c3dc366e1a88c3607b4...master

Your commit was automatically rebased without conflicts.

Pushed as commit 3c4e824.

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

8249783: Simplify DerValue and DerInputStream

6f63d1a

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

openjdk bot added the security security-dev@openjdk.org label Sep 17, 2020

Enhance DerValue::getOctetString to be able to read multi-level const…

e8f804c

…ructed value.

valeriepeng reviewed Sep 25, 2020

View reviewed changes

wangweij commented Sep 26, 2020

View reviewed changes

wangweij added 2 commits September 29, 2020 11:13

Merge remote-tracking branch 'origin/master' into 8249783

fdf3094

8249783: Simplify DerValue and DerInputStream

ac77172

valeriepeng reviewed Sep 30, 2020

View reviewed changes

valeriepeng approved these changes Oct 1, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 1, 2020

some small tuning

2320563

openjdk bot closed this Oct 1, 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 Oct 1, 2020

wangweij deleted the 8249783 branch October 1, 2020 19:01

8249783: Simplify DerValue and DerInputStream #232

8249783: Simplify DerValue and DerInputStream #232

Conversation

wangweij commented Sep 17, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 17, 2020

openjdk bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020 • edited

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

valeriepeng Sep 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij commented Sep 29, 2020 • edited

Choose a reason for hiding this comment

valeriepeng left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 1, 2020 • edited

wangweij commented Oct 1, 2020

wangweij commented Oct 1, 2020

openjdk bot commented Oct 1, 2020

wangweij commented Oct 1, 2020

openjdk bot commented Oct 1, 2020

wangweij commented Sep 17, 2020 •

edited by openjdk bot

mlbridge bot commented Sep 17, 2020 •

edited

valeriepeng Sep 29, 2020 •

edited

wangweij commented Sep 29, 2020 •

edited

openjdk bot commented Oct 1, 2020 •

edited