8376748: Emit runtime warnings for JCE algorithms that will be disabled#29719
Draft
seanjmullan wants to merge 28 commits intoopenjdk:masterfrom
Draft
8376748: Emit runtime warnings for JCE algorithms that will be disabled#29719seanjmullan wants to merge 28 commits intoopenjdk:masterfrom
seanjmullan wants to merge 28 commits intoopenjdk:masterfrom
Conversation
Update copyright year, improve comments and use File::toPath to convert back to Path.
Junctions do not require elevation, so this is a way of testing
soft-links are resolved without requiring elevation. This is useful
because we need to avoid elevation in order to reproduce the parent
directories permission issue.
This is testing directories structure:
📁 JDK-8352728-tmp-*/
├─🔒 jdk-parent-dir/ (ACL with REMOVED-PERMISSIONS)
│ └─📁 jdk/
│ ├─📁 conf/
│ │ ├─📁 security/
│ │ │ ├─📄 java.security
│ │ │ │ 📝 include link-to-other-dir/other.properties
│ │ │ ├─🔗 link-to-other-dir/ ⟹ 📁 JDK-8352728-tmp-*/other-dir
│ │ │ └─... (JUNCTION)
│ │ └─...
│ └─...
├─📁 other-dir/
│ └─📄 other.properties
│ 📝 include ../relatively.included.properties
└─📄 relatively.included.properties
📝 test.property.name=test_property_value
This use case has been discussed and analyzed in the pull request, but we didn't have a test case for it. By introducing a test, we make sure we don't have regressions in this area, regardless of the alternative we choose to advance with for this fix.
checkCyclicInclude() is invoked after we successfully get an InputStream for the path to avoid skipping the same IOException several times inside checkCyclicInclude() if the path doesn't exist. Also, perform symlinks resolution only in the following cases: • When we need to resolve a relative include • For clarity to the user in logging messages • For clarity to the user in exception messages In the first case, the resolution is a requirement, in the last two cases it is a nice-to-have. But given the last two are exceptional cases anyway, we let any resolution error bubble up.
OutputAnalyzer::stderrMatches returns a boolean while OutputAnalyzer::stderrShouldMatch performs the check.
Remove path normalization from debugging messages, only keep it where strictly necessary. Adjust test case to the new debugging messages which contain absolute but not normalized paths (can contain some ../../ elements).
After JDK-8355342 (1f08a3e) File::getCanonicalFile is no longer resolving symlinks in Windows, plus there is some reluctance from core-libs maintainers about relying on specific java.io.File behaviour with regard to parent directories permissions. On the other hand, path resolution has been moved to a place where it's strictly needed (when resolving a relative include directive), and this should mitigate the majority of the use-cases, being an improvement with respect to the current status quo. Given the previous, I think that Path::toRealPath in its current location is a good compromise. This also reverts commit 7abb62c, since we are no longer supporting that use-case.
Extract restrictedAcl() AutoCloseable and also use AutoCloseable for the temporary directory cleanup.
We agreed an IOException in this case is recoverable, and decided to tolerate it, while adding a debug log message with the exception.
As @wangweij pointed out: > The person writing the original properties file may have expected > includes to resolve relative to its own location, but whoever created > the symlink may have intended a different resolution path. If they > wanted the original location, they could have just used the real file > directly instead of introducing a symlink. Since path resolution is causing trouble under certain conditions, let's avoid doing it. Other programs with include directives support in their configuration files are doing the same.
This reverts commit 7c80874 only for ConfigFileTest.
Move ExtraMode as an ExtraPropsFile field, so we have this information as soon as the ExtraPropsFile is created. This will be useful for the next change.
|
👋 Welcome back mullan! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@seanjmullan The following label will be automatically applied to this pull request:
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Progress
Error
Integration blocker
8376748. Please make sure it exists and is accessible.Issue
8376748.Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/29719/head:pull/29719$ git checkout pull/29719Update a local copy of the PR:
$ git checkout pull/29719$ git pull https://git.openjdk.org/jdk.git pull/29719/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 29719View PR using the GUI difftool:
$ git pr show -t 29719Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/29719.diff