Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8266019: StreamResult(File) writes to incorrect file path if # is part of the file path #4318

Closed
wants to merge 1 commit into from

Conversation

@JoeWang-Java
Copy link
Member

@JoeWang-Java JoeWang-Java commented Jun 2, 2021

Special characters are different in File and URI. Treat File input as File using FileInputStream instead of converting to an URI, but fall back to URI in case of error for compatibility (in error handling).


Progress

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

Issue

  • JDK-8266019: StreamResult(File) writes to incorrect file path if # is part of the file path

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4318

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jun 2, 2021

👋 Welcome back joehw! 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 openjdk bot added the rfr label Jun 2, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jun 2, 2021

@JoeWang-Java 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 2, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 2, 2021

Webrevs

@dfuch
Copy link
Member

@dfuch dfuch commented Jun 2, 2021

I believe this is the wrong fix - or at least an incomplete fix that will hide the bug under the carpet. Looking at TransformImpl I would change line TransformImpl.java:521 to:

521: -     String path = uri.getPath(); //decoded String
521: +     String path = uri.getRawPath(); //raw String

@JoeWang-Java
Copy link
Member Author

@JoeWang-Java JoeWang-Java commented Jun 2, 2021

I tried, but getRawPath returned output/%23/dom.xml, that in turn resulted in java.io.FileNotFoundException later.

For StreamResult, when a String-form systemId is in File protocol, a FileOutputStream will eventually get created. Since StreamResult accepts OutputStream, there's no need for the File -> URI -> systemId -> URL -> File conversion (as did in TransformerImpl). I think it's better to create a FileOutputStream right from the File input.

@dfuch
Copy link
Member

@dfuch dfuch commented Jun 2, 2021

Argh! You're right. It's this line which is horribly wrong:

_ostream = new FileOutputStream(url.getFile());

OK - hopefully with your fix this becomes dead code?

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

@dfuch dfuch left a comment

I now agree that what you propose is the safer route to fix this particular issue.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 2, 2021

@JoeWang-Java 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:

8266019: StreamResult(File) writes to incorrect file path if # is part of the file path

Reviewed-by: dfuchs

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

  • 375f8f3: 8268138: docs build error after JDK-8263332 integration
  • a8835b9: 8267879: ClassLoaderMetaspace destructor asserts on !_frozen
  • ecf6112: 8267958: [TESTBUG] cds DynamicLoaderConstraintsTest.java timed out
  • 1ae934e: 8263332: JFR: Dump recording from a recording stream
  • b7ac705: 8263642: javac emits duplicate checkcast for first bound of intersection type in cast
  • e1462e7: 8267176: StandardDoclet should provide access to Reporter and Locale

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 Jun 2, 2021
@JoeWang-Java
Copy link
Member Author

@JoeWang-Java JoeWang-Java commented Jun 2, 2021

Thanks Daniel!

For the question, it will not be dead code as it accepts systemId in String form as well. But as least when File is the input, we don't end up going through a convoluted (if not unnecessary) File -URI -URL - File conversion. I think the later has sth. to do with the fact that Apache still supports older JDKs. While we agree not to touch that code for this fix (and also for not changing the mechanisms, e.g. how errors are handled), we'll keep an eye out on it.

@JoeWang-Java
Copy link
Member Author

@JoeWang-Java JoeWang-Java commented Jun 3, 2021

/integrate

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

@openjdk openjdk bot commented Jun 3, 2021

@JoeWang-Java Since your change was applied there have been 32 commits pushed to the master branch:

  • b955865: 8267995: Add reference to JVMS class file format in Lookup::defineHiddenClass
  • 9f05c41: 8265783: Create a separate library for x86 Intel SVML assembly intrinsics
  • e27c4d4: 8268185: Update GitHub Actions for jtreg 6
  • 68ac871: 8268189: ProblemList compiler/intrinsics/bmi/verifycode/BzhiTestI2L.java in -Xcomp mode
  • af3df63: 8267598: jpackage removes system libraries from java.library.path
  • 52d8215: 8268131: 2 java/foreign tests timed out
  • 5405f98: 8268077: java.util.List missing from Collections Framework Overview
  • 3aa7062: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"
  • 5982cfc: 8266317: Vector API enhancements
  • eb385c0: 8268167: MultipleLogins.java failure on macosx-aarch64
  • ... and 22 more: https://git.openjdk.java.net/jdk/compare/56b65e4a8d519801d170e16063ccb7dd3069c4be...master

Your commit was automatically rebased without conflicts.

Pushed as commit 460ce55.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants