8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field #9306

jhuttana · 2022-06-28T08:33:58Z

Could you please review the changes?
This is to address the issue: https://bugs.openjdk.org/browse/JDK-8283082?jql=labels%20%3D%20starter-bug

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field

Reviewers

Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9306/head:pull/9306
$ git checkout pull/9306

Update a local copy of the PR:
$ git checkout pull/9306
$ git pull https://git.openjdk.org/jdk pull/9306/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9306

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9306.diff

…nulls out info field

bridgekeeper · 2022-06-28T08:35:02Z

👋 Welcome back jhuttana! 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 · 2022-06-28T08:39:41Z

@jhuttana 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 pull request command.

mlbridge · 2022-06-28T08:44:03Z

Webrevs

wangweij · 2022-06-28T13:17:42Z

src/java.base/share/classes/sun/security/x509/X509CertImpl.java


        if (id.equalsIgnoreCase(INFO)) {
-            if (attr.getSuffix() != null) {
+            if (!(attr.getSuffix() != null)) {


You can simply update the != to ==.

Also, please add a regression test. Since this is about testing an internal method, you are free to add @modules java.base/sun.security.x509 to access it.

The test can be much simpler than the program included in the bug report. Something like this:

Create an empty X509CertImpl

Add "x509.info"

Add "x509.info.issuer"

Remove "x509.info.issuer"

Add "x509.info.issuer" again

Without the fix, step 4 removes the whole info and step 5 would fail.

Thank you for taking a look at the PR.
Sure I will address these comments.

The test can be much simpler than the program included in the bug report. Something like this:

1. Create an empty `X509CertImpl` 2. Add "x509.info" 3. Add "x509.info.issuer" 4. Remove "x509.info.issuer" 5. Add "x509.info.issuer" again

Without the fix, step 4 removes the whole info and step 5 would fail.
I am not a getting a clear idea about how to add issuer and delete etc.
Could you please point me to some example?

import sun.security.x509.X500Name; import sun.security.x509.X509CertImpl; import sun.security.x509.X509CertInfo; public class A { public static void main(String[] args) throws Exception { var c = new X509CertImpl(); c.set("x509.info", new X509CertInfo()); c.set("x509.info.issuer", new X500Name("CN=one")); c.delete("x509.info.issuer"); c.set("x509.info.issuer", new X500Name("CN=two")); } }

Thank you for providing this!

wangweij · 2022-06-29T13:44:11Z

src/java.base/share/classes/sun/security/x509/X509CertImpl.java


        if (id.equalsIgnoreCase(INFO)) {
-            if (attr.getSuffix() != null) {
+            if ((attr.getSuffix() == null)) {


You can remove the parentheses that were added last time.

Sure I will do that :)

@wangweij I have added the test case under: test/jdk/sun/security/x509/X509CertImpl/JDK8283082.java
As this is the first I am adding regression test so I am not sure whether the naming convention for the test case is right or not.

Thanks for adding the test. It looks fine. The name is good.

Please add a copyright header and a @test block comment. You can find a nearby existing test file and see what they look like. Since the new test references internal classes, a @modules java.base/sun.security.x509 is necessary. Please run the test with the jtreg command to ensure everything is correct. Before your fix, the test should fail; and after it, it should succeed.

Thanks for adding the test. It looks fine. The name is good.

Please add a copyright header and a @test block comment. You can find a nearby existing test file and see what they look like. Since the new test references internal classes, a @modules java.base/sun.security.x509 is necessary. Please run the test with the jtreg command to ensure everything is correct. Before your fix, the test should fail; and after it, it should succeed.

Ok Sure :) Thanks for suggesting this.

jhuttana · 2022-07-07T10:23:36Z

I tried to look at what is cause for the failure and not able to locate the detail !!
I navigated till here: https://github.com/jhuttana/jdk/runs/7116877356?check_suite_focus=true but nothing was useful to find the details.
Could you please guide me where I should look at the details?

wangweij · 2022-07-07T13:58:53Z

You can expand the "Run Tests" task and see the failed one (it's very long). It's a linux-x86 test and it mentions "thread". I've heard that thread-related tests could fail on linux-x86 because loom support was only added to linux-x64. Unless you heard from someone else that it's a real issue, I think you can ignore it.

jhuttana · 2022-07-07T15:36:07Z

You can expand the "Run Tests" task and see the failed one (it's very long). It's a linux-x86 test and it mentions "thread". I've heard that thread-related tests could fail on linux-x86 because loom support was only added to linux-x64. Unless you heard from someone else that it's a real issue, I think you can ignore it.

Sure I will check and confirm that. Thank you!

- Verified the newly added test case before and after the patch.

jhuttana · 2022-07-08T09:04:06Z

I made the suggested changes to the test case and verified before and after the patch with jtreg.
Steps followed are as below:

export JTREG_JAVA=/usr/bin/java

Before Patch:

jtreg/bin/jtreg -jdk:/root/jdk/build.master/linux-x86_64-server-release/images/jdk/ test/jdk/sun/security/x509/X509CertImpl/
Test results: passed: 5; failed: 1
Report written to /root/jdk/JTreport/html/report.html
Results written to /root/jdk/JTwork
Error: Some tests failed or other problems occurred.

# cat /root/jdk/JTwork/jtData/harness.trace
# Trace file started at 2022-07-08T04:47:29-0400
# JT Harness version 6.0
# class directory: /root/jdk/jtreg/lib/javatest.jar
# using java: /usr/lib/jvm/java-18-openjdk-18.0.1.0.10-1.rolling.fc36.x86_64
2022-07-08T04:47:29-0400 Starting tests.
Test suite class: com.sun.javatest.regtest.config.RegressionTestSuite
Test finder class: com.sun.javatest.regtest.config.RegressionTestFinder
2022-07-08T04:47:30-0400 Test starting: sun/security/x509/X509CertImpl/CertificateValidation.java
2022-07-08T04:47:30-0400 Test finished: sun/security/x509/X509CertImpl/CertificateValidation.java: Passed. Execution successful
2022-07-08T04:47:30-0400 Test starting: sun/security/x509/X509CertImpl/ECSigParamsVerifyWithCert.java
2022-07-08T04:47:31-0400 Test finished: sun/security/x509/X509CertImpl/ECSigParamsVerifyWithCert.java: Passed. Execution successful
2022-07-08T04:47:31-0400 Test starting: sun/security/x509/X509CertImpl/GetFingerprintError.java
2022-07-08T04:47:31-0400 Test finished: sun/security/x509/X509CertImpl/GetFingerprintError.java: Passed. Execution successful
2022-07-08T04:47:31-0400 Test starting: sun/security/x509/X509CertImpl/JDK8283082.java
2022-07-08T04:47:31-0400 Test finished: sun/security/x509/X509CertImpl/JDK8283082.java: Failed. Execution failed: `main' threw exception: java.lang.NullPointerException: Cannot invoke "sun.security.x509.X509CertInfo.set(String, Object)" because "this.info" is null
2022-07-08T04:47:31-0400 Test starting: sun/security/x509/X509CertImpl/V3Certificate.java
2022-07-08T04:47:32-0400 Test finished: sun/security/x509/X509CertImpl/V3Certificate.java: Passed. Execution successful
2022-07-08T04:47:32-0400 Test starting: sun/security/x509/X509CertImpl/Verify.java
2022-07-08T04:47:32-0400 Test finished: sun/security/x509/X509CertImpl/Verify.java: Passed. Execution successful
2022-07-08T04:47:32-0400 Doing post-run cleanup
2022-07-08T04:47:32-0400 Completed: Some tests did not pass

After Patch:

jtreg/bin/jtreg -jdk:/root/jdk/build.8283082/linux-x86_64-server-release/images/jdk/ test/jdk/sun/security/x509/X509CertImpl/
Directory "JTwork" not found: creating
Directory "JTreport" not found: creating
Test results: passed: 6
Report written to /root/jdk/JTreport/html/report.html
Results written to /root/jdk/JTwork

# cat /root/jdk/JTwork/jtData/harness.trace
# Trace file started at 2022-07-08T04:44:58-0400
# JT Harness version 6.0
# class directory: /root/jdk/jtreg/lib/javatest.jar
# using java: /usr/lib/jvm/java-18-openjdk-18.0.1.0.10-1.rolling.fc36.x86_64
2022-07-08T04:44:58-0400 Starting tests.
Test suite class: com.sun.javatest.regtest.config.RegressionTestSuite
Test finder class: com.sun.javatest.regtest.config.RegressionTestFinder
2022-07-08T04:44:58-0400 Test starting: sun/security/x509/X509CertImpl/CertificateValidation.java
2022-07-08T04:44:59-0400 Test finished: sun/security/x509/X509CertImpl/CertificateValidation.java: Passed. Execution successful
2022-07-08T04:44:59-0400 Test starting: sun/security/x509/X509CertImpl/ECSigParamsVerifyWithCert.java
2022-07-08T04:45:00-0400 Test finished: sun/security/x509/X509CertImpl/ECSigParamsVerifyWithCert.java: Passed. Execution successful
2022-07-08T04:45:00-0400 Test starting: sun/security/x509/X509CertImpl/GetFingerprintError.java
2022-07-08T04:45:01-0400 Test finished: sun/security/x509/X509CertImpl/GetFingerprintError.java: Passed. Execution successful
2022-07-08T04:45:01-0400 Test starting: sun/security/x509/X509CertImpl/JDK8283082.java
2022-07-08T04:45:02-0400 Test finished: sun/security/x509/X509CertImpl/JDK8283082.java: Passed. Execution successful
2022-07-08T04:45:02-0400 Test starting: sun/security/x509/X509CertImpl/V3Certificate.java
2022-07-08T04:45:04-0400 Test finished: sun/security/x509/X509CertImpl/V3Certificate.java: Passed. Execution successful
2022-07-08T04:45:04-0400 Test starting: sun/security/x509/X509CertImpl/Verify.java
2022-07-08T04:45:05-0400 Test finished: sun/security/x509/X509CertImpl/Verify.java: Passed. Execution successful
2022-07-08T04:45:05-0400 Doing post-run cleanup
2022-07-08T04:45:05-0400 Completed: All tests passed

So, this confirms that our test case is exercising the changes and producing the right results.
Could you please have a look at the recent changes?

wangweij · 2022-07-08T13:01:16Z

Great, this looks good. Please use 2022 in the copyright year of the test. You might also want to use your own company's name since this is a new test. See https://github.com/openjdk/jdk/blob/1ee80e03adfae5f428519f7c134e78a0f277a0a5/test/jdk/sun/security/pkcs11/Cipher/EncryptionPadding.java for an example or ask a colleague on what the correct words should be.

jhuttana · 2022-07-08T13:28:36Z

Great, this looks good. Please use 2022 in the copyright year of the test. You might also want to use your own company's name since this is a new test. See https://github.com/openjdk/jdk/blob/1ee80e03adfae5f428519f7c134e78a0f277a0a5/test/jdk/sun/security/pkcs11/Cipher/EncryptionPadding.java for an example or ask a colleague on what the correct words should be.

Sure. Thank you.

jhuttana · 2022-07-12T14:31:54Z

@wangweij I have made the suggested changes to test case copyright section.
Thank you so much for reviewing this PR and guiding at every stage.

@jerboaa Thank you for suggesting the suitable changes to the test case.

wangweij

Everything looks fine to me. Thanks.

openjdk · 2022-07-12T18:16:07Z

@jhuttana 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:

8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field

Reviewed-by: weijun

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

d9ca438: Merge
3164c98: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM
c3806b9: 8289709: fatal error: stuck in JvmtiVTMSTransitionDisabler::disable_VTMS_transitions
39715f3: 8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows
62fbc3f: 8287379: Using @inheritdoc in an inapplicable context shouldn't crash javadoc
fed3af8: 8287809: Revisit implementation of memory session
e8568b8: 8290020: Deadlock in leakprofiler::emit_events during shutdown
7f0e9bd: 8289745: JfrStructCopyFailed uses heap words instead of bytes for object sizes
728157f: 8289917: Metadata for regionsRefilled of G1EvacuationStatistics event is wrong
0e90697: 8290133: JFR: Remove unused methods in Bits.java
... and 221 more: https://git.openjdk.org/jdk/compare/7905788e969727c81eea4397f0d9b918cdb5286a...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 (@wangweij) 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).

jhuttana · 2022-07-12T18:28:12Z

/integrate

openjdk · 2022-07-12T18:29:27Z

@jhuttana An unexpected error occurred during integration. No push attempt will be made. The error has been logged and will be investigated. It is possible that this error is caused by a transient issue; feel free to retry the operation.

jhuttana · 2022-07-12T18:31:44Z

/integrate

openjdk · 2022-07-12T18:32:19Z

@jhuttana An unexpected error occurred during integration. No push attempt will be made. The error has been logged and will be investigated. It is possible that this error is caused by a transient issue; feel free to retry the operation.

jhuttana · 2022-07-12T18:34:28Z

@wangweij Thanks for approving the PR.
As per instruction I typed /integrate and not sure why it is struck at some unexpected error!

wangweij · 2022-07-12T18:35:50Z

I haven't seen this error before. Try again.

jhuttana · 2022-07-12T19:12:31Z

I haven't seen this error before. Try again.

Oh okay :) Sure I will try again

jhuttana · 2022-07-12T19:12:51Z

/integrate

openjdk · 2022-07-12T19:13:18Z

@jhuttana An unexpected error occurred during integration. No push attempt will be made. The error has been logged and will be investigated. It is possible that this error is caused by a transient issue; feel free to retry the operation.

jhuttana · 2022-07-12T20:03:35Z

/integrate

openjdk · 2022-07-12T20:04:53Z

@jhuttana
Your change (at version 54ce3cd) is now ready to be sponsored by a Committer.

wangweij · 2022-07-12T20:06:57Z

/sponsor

jhuttana · 2022-07-12T20:07:16Z

@wangweij Finally it worked somehow :)
Thank you!

openjdk · 2022-07-12T20:08:29Z

@wangweij @jhuttana An unexpected error occurred during sponsored integration. No push attempt will be made. The error has been logged and will be investigated. It is possible that this error is caused by a transient issue; feel free to retry the operation.

wangweij · 2022-07-12T20:11:10Z

/sponsor

openjdk · 2022-07-12T20:12:30Z

Going to push as commit 31f7fc0.
Since your change was applied there have been 231 commits pushed to the master branch:

d9ca438: Merge
3164c98: 8289148: j.l.foreign.VaList::nextVarg call could throw IndexOutOfBoundsException or even crash the VM
c3806b9: 8289709: fatal error: stuck in JvmtiVTMSTransitionDisabler::disable_VTMS_transitions
39715f3: 8287902: UnreadableRB case in MissingResourceCauseTest is not working reliably on Windows
62fbc3f: 8287379: Using @inheritdoc in an inapplicable context shouldn't crash javadoc
fed3af8: 8287809: Revisit implementation of memory session
e8568b8: 8290020: Deadlock in leakprofiler::emit_events during shutdown
7f0e9bd: 8289745: JfrStructCopyFailed uses heap words instead of bytes for object sizes
728157f: 8289917: Metadata for regionsRefilled of G1EvacuationStatistics event is wrong
0e90697: 8290133: JFR: Remove unused methods in Bits.java
... and 221 more: https://git.openjdk.org/jdk/compare/7905788e969727c81eea4397f0d9b918cdb5286a...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-07-12T20:12:45Z

@wangweij @jhuttana Pushed as commit 31f7fc0.

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

8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") …

39c33a7

…nulls out info field

openjdk bot changed the title ~~8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") …~~ 8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field Jun 28, 2022

openjdk bot added the rfr Pull request is ready for review label Jun 28, 2022

openjdk bot added the security security-dev@openjdk.org label Jun 28, 2022

wangweij reviewed Jun 28, 2022

View reviewed changes

Address review comments

e93be28

wangweij reviewed Jun 29, 2022

View reviewed changes

Address review comments. Add regression test for the change

5d89e9b

Address review comment to polish regression test.

fcafa9d

- Verified the newly added test case before and after the patch.

openjdk bot removed the rfr Pull request is ready for review label Jul 8, 2022

Address trailing whitespace reported by jcheck in GHA

d74fedd

openjdk bot added the rfr Pull request is ready for review label Jul 8, 2022

Address review comments to update copyright section.

54ce3cd

wangweij approved these changes Jul 12, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 12, 2022

openjdk bot added the sponsor Pull request is ready to be sponsored label Jul 12, 2022

openjdk bot added the integrated Pull request has been integrated label Jul 12, 2022

openjdk bot closed this Jul 12, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jul 12, 2022

8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field #9306

8283082: sun.security.x509.X509CertImpl.delete("x509.info.validity") nulls out info field #9306

Uh oh!

Conversation

jhuttana commented Jun 28, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jun 28, 2022

Uh oh!

openjdk bot commented Jun 28, 2022

Uh oh!

mlbridge bot commented Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhuttana commented Jul 7, 2022

Uh oh!

wangweij commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhuttana commented Jul 7, 2022

Uh oh!

jhuttana commented Jul 8, 2022

Uh oh!

wangweij commented Jul 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhuttana commented Jul 8, 2022

Uh oh!

jhuttana commented Jul 12, 2022

Uh oh!

wangweij left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhuttana commented Jul 12, 2022

Uh oh!

openjdk bot commented Jul 12, 2022

Uh oh!

jhuttana commented Jul 12, 2022

Uh oh!

openjdk bot commented Jul 12, 2022

Uh oh!

jhuttana commented Jul 12, 2022

Uh oh!

wangweij commented Jul 12, 2022

Uh oh!

jhuttana commented Jul 12, 2022

jhuttana commented Jun 28, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 28, 2022 •

edited

Loading

wangweij Jun 28, 2022 •

edited

Loading

wangweij Jul 7, 2022 •

edited

Loading

wangweij commented Jul 7, 2022 •

edited

Loading

wangweij commented Jul 8, 2022 •

edited

Loading

openjdk bot commented Jul 12, 2022 •

edited

Loading