Skip to content
This repository has been archived by the owner. It is now read-only.

8268826: Cleanup Override in Context-Specific Deserialization Filters #85

Closed
wants to merge 7 commits into from
Closed

8268826: Cleanup Override in Context-Specific Deserialization Filters #85

wants to merge 7 commits into from

Conversation

RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Jun 16, 2021

Remove the unnecessary special case "OVERRIDE" in jdk.serialFilterFactory property.
Fix description in the example of a filter allowing platform classes.
Suppress some warnings about use of SecurityManager in tests.


Progress

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

Issue

  • JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 85

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

Using diff file

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

JDK-8268615: (doc) Eratta in javadoc for ObjectInputFilter Example
@RogerRiggs
Copy link
Contributor Author

RogerRiggs commented Jun 16, 2021

/issue JDK-8268615 : (doc) Eratta in javadoc for ObjectInputFilter Example

@RogerRiggs
Copy link
Contributor Author

RogerRiggs commented Jun 16, 2021

/csr

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 16, 2021

👋 Welcome back rriggs! 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.

@RogerRiggs RogerRiggs marked this pull request as ready for review Jun 16, 2021
@openjdk openjdk bot added the rfr label Jun 16, 2021
@openjdk
Copy link

openjdk bot commented Jun 16, 2021

@RogerRiggs
Adding additional issue to issue list: 8268615: (doc) Eratta in javadoc for ObjectInputFilter Example.

@openjdk openjdk bot added the csr label Jun 16, 2021
@openjdk
Copy link

openjdk bot commented Jun 16, 2021

@RogerRiggs this pull request will not be integrated until the CSR request JDK-8268827 for issue JDK-8268826 has been approved.

@openjdk
Copy link

openjdk bot commented Jun 16, 2021

@RogerRiggs 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 Jun 16, 2021
@mlbridge
Copy link

mlbridge bot commented Jun 16, 2021

dfuch
dfuch approved these changes Jun 17, 2021
Copy link
Member

@dfuch dfuch left a comment

Looks reasonable to me.

Copy link
Member

@bchristi-git bchristi-git left a comment

OVERRIDE is also mentioned in src/java.base/share/conf/security/java.security b/src/java.base/share/conf/security/java.security

Correct description of the example of allowing platform or bootstrap classes in OIF.
Cleanup of logging of filter tracing.
RogerRiggs added 2 commits Jun 22, 2021
by ensuring that Config.setSerialFilterFactory throws IllegalStateException
if called as a side effect of class loading and initialization.
is misconfigured and test those faults.
Fix typo in java.security-extra-factory test config
configLog.log(ERROR,
"Error configuring filter: {0}", re);
Copy link
Member

@dfuch dfuch Jun 24, 2021

Choose a reason for hiding this comment

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

You should either remove "{0}" from the string, or cast re to Object - as there is an overridden log method that takes a Throwable. If you want to display the exception stack trace, just call:

configLog.log(ERROR, "Error configuring filter", re);

If you don't want to display the stack trace, call:

configLog.log(ERROR, "Error configuring filter: {0}", (Object)re);

Copy link
Contributor Author

@RogerRiggs RogerRiggs Jun 24, 2021

Choose a reason for hiding this comment

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

Hard to spot that mis-use.
Also observed that if the jdk.serialFilter is ill formed (IAE), it gets logged but should also rethrow
the exception to prevent continuing without an expected filter.
I'll file a separate bug since it predates this PR.

…xception message

Simplify the logging.properties to only the needed settings
dfuch
dfuch approved these changes Jun 25, 2021
System.setProperty("java.util.logging.config.file",
System.getProperty("test.src", ".") + "/logging.properties");
Copy link
Member

@bchristi-git bchristi-git Jun 25, 2021

Choose a reason for hiding this comment

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

Is System.setProperty() needed if it's already being set with -D ?

Copy link
Contributor Author

@RogerRiggs RogerRiggs Jun 25, 2021

Choose a reason for hiding this comment

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

Fooey; belt and suspenders not needed here.
In some cases, I used the static initializer to avoid adding command line arguments that would
obscure the command line arguments being tested.
In this case, the log may be interesting for debugging but not integral to the test correctness.

…tation

Test cleanup; enable logging of serialization on the command line (remove static)
Remove obsolete comment about jdk.serialFilter
Fix typo in StaticProperty.java
@openjdk openjdk bot removed the csr label Jul 8, 2021
@RogerRiggs RogerRiggs changed the title JDK-8268826: Cleanup Override in Context-Specific Deserialization Filters 8268826: Cleanup Override in Context-Specific Deserialization Filters Jul 8, 2021
@RogerRiggs
Copy link
Contributor Author

RogerRiggs commented Jul 9, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Jul 9, 2021

@RogerRiggs This PR has not yet been marked as ready for integration.

@RogerRiggs
Copy link
Contributor Author

RogerRiggs commented Jul 9, 2021

/issue remove JDK-8268615

it didn't like the title difference

@openjdk
Copy link

openjdk bot commented Jul 9, 2021

@RogerRiggs
Removing additional issue from issue list: 8268615.

@openjdk
Copy link

openjdk bot commented Jul 9, 2021

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

8268826: Cleanup Override in Context-Specific Deserialization Filters

Reviewed-by: dfuchs, bchristi

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

  • f791fdf: 8261147: C2: Node is wrongly marked as reduction resulting in a wrong execution due to wrong vector instructions
  • 1196b35: 8270151: IncompatibleClassChangeError on empty pattern switch statement case
  • 885f7b1: 8269146: Missing unreported constraints on pattern and other case label combination
  • 62ff55d: 8269952: compiler/vectorapi/VectorCastShape*Test.java tests failed on avx2 machines
  • 46c610c: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms
  • 6401633: 8269722: NPE in HtmlDocletWriter
  • 9acb2a6: 8270109: ProblemList 4 SA tests on macOS-aarch64
  • f46a917: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF
  • 9e75f92: 8269738: AssertionError when combining pattern matching and function closure
  • 168af2e: 8269828: corrections in some instruction patterns for KNL x86 platform
  • ... and 157 more: https://git.openjdk.java.net/jdk17/compare/9ad19f7838e6f6e128583c191c5507c1e2bd5083...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.

@openjdk openjdk bot added the ready label Jul 9, 2021
@RogerRiggs
Copy link
Contributor Author

RogerRiggs commented Jul 9, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Jul 9, 2021

Going to push as commit 6889a39.
Since your change was applied there have been 167 commits pushed to the master branch:

  • f791fdf: 8261147: C2: Node is wrongly marked as reduction resulting in a wrong execution due to wrong vector instructions
  • 1196b35: 8270151: IncompatibleClassChangeError on empty pattern switch statement case
  • 885f7b1: 8269146: Missing unreported constraints on pattern and other case label combination
  • 62ff55d: 8269952: compiler/vectorapi/VectorCastShape*Test.java tests failed on avx2 machines
  • 46c610c: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms
  • 6401633: 8269722: NPE in HtmlDocletWriter
  • 9acb2a6: 8270109: ProblemList 4 SA tests on macOS-aarch64
  • f46a917: 6766844: ByteArrayInputStream#read with a byte array of length 0 not consistent with InputStream when at EOF
  • 9e75f92: 8269738: AssertionError when combining pattern matching and function closure
  • 168af2e: 8269828: corrections in some instruction patterns for KNL x86 platform
  • ... and 157 more: https://git.openjdk.java.net/jdk17/compare/9ad19f7838e6f6e128583c191c5507c1e2bd5083...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 9, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 9, 2021
@openjdk
Copy link

openjdk bot commented Jul 9, 2021

@RogerRiggs Pushed as commit 6889a39.

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

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