8276050: XMLInputFactoryImpl.getProperty() returns null #8063

JoeWang-Java · 2022-03-31T19:37:08Z

Fix a bug that missed checking security property manager for such properties.

Progress

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

Issue

JDK-8276050: XMLInputFactoryImpl.getProperty() returns null

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8063

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

Using diff file

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

bridgekeeper · 2022-03-31T19:38:40Z

👋 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 · 2022-03-31T19:40:46Z

@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.

mlbridge · 2022-03-31T19:45:05Z

Webrevs

01: Full - Incremental (c6b741c3)
00: Full (7419095b)

turbanoff · 2022-03-31T20:49:53Z

src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/PropertyManager.java

@@ -171,6 +171,11 @@ public Object getProperty(String property){
        /** Check to see if the property is managed by the security manager **/
        String propertyValue = (fSecurityManager != null) ?
                fSecurityManager.getLimitAsString(property) : null;
+        /** Check to see if the property is managed by the security property manager **/
+        if (propertyValue == null){


Let's add space before {

Added. Also hit the format button for the class, that stirred up a hornet's nest, ended up making more (unnecessary) changes. Fortunately, it's confined to format and comments (not affecting the functionality).

Thanks, I didn't see the split in the commits.
IMHO for an existing class, its a bad idea to ask an IDE to reformat a whole file; just too many gratuitous changes.

Very true. When you look at these legacy code, it's quite tempting as you make style changes, e.g. adding a space next to more such things in the adjacent code. But yeh, must resist the urge :-)

Thanks again!

RogerRiggs

It would be a big help if the style cleanup was done separately or after the
intended change was reviewed. It adds a lot of noise the the commit.
The real change is buried in there somewhere but looks ok.

JoeWang-Java · 2022-04-01T22:21:14Z

It would be a big help if the style cleanup was done separately or after the intended change was reviewed. It adds a lot of noise the the commit. The real change is buried in there somewhere but looks ok.

I agree. It wasn't the original attention, normally shouldn't touch the legacy code or style. So thanks for tolerating the noise.

JoeWang-Java · 2022-04-01T22:24:26Z

Review note: please note that the fix is in the initial commit. The 2nd note contain code cleanup only, no code or functional change. Sorry about the noise.

openjdk · 2022-04-01T23:44:52Z

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

8276050: XMLInputFactoryImpl.getProperty() returns null

Reviewed-by: rriggs

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

afd0f5a: 8284190: disable G1RegionToSpaceMapper.largeStressAdjacent_vm on windows
028fbf4: 8254935: Deprecate the PSSParameterSpec(int) constructor
1dfa1ea: 8284094: Memory leak in invoker_completeInvokeRequest()
943d4ee: 8284180: Some files missing newlines
96179c8: 8284146: Disable jdk/jshell/HighlightUITest.java on macosx-aarch64
63fec5d: 8283525: http://tools.ietf.org/html/* URLs return 404
dbfac3c: 8283269: Improve definition and use of jdk.javadoc.internal.doclets.toolkit.Content
fc7a17c: 8284103: AsVarargsCollector::asCollectorCache incorrectly marked @stable
05ea7a5: 8282431: AArch64: Add optimized rules for masked vector multiply-add/sub for SVE
9156c0b: 8274148: can jshell show deprecated classes, methods and fields as strikethrough text?
... and 3 more: https://git.openjdk.java.net/jdk/compare/ad83ec7e281cb3ab7c42e71fab47ea21b93079ea...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.

JoeWang-Java · 2022-04-04T19:58:55Z

/integrate

openjdk · 2022-04-04T20:00:51Z

Going to push as commit 61d06c2.
Since your change was applied there have been 24 commits pushed to the master branch:

7381868: 8283623: Create an automated regression test for JDK-4525475
f76f5da: 5087440: java.io bulk read(...) end-of-stream return value descriptions ambiguous
36b9baa: 8282508: Updating ASM to 9.2 for JDK 19
4e20a03: 8283044: Use asynchronous handshakes to deliver asynchronous exceptions
9d200d6: 8282756: Make ElementKind checks more specific
1012d59: 8281079: [s390] Unify Address Operand Encoding in Instruction Emitters
003ec21: 8279508: Auto-vectorize Math.round API
c1e67b6: 8283474: Include detailed heap object info in CDS map file
060a188: 8283865: riscv: Break down -XX:+UseRVB into seperate options for each bitmanip extension
e5e1aab: 8284068: riscv: should call Atomic::release_store in JavaThread::set_thread_state
... and 14 more: https://git.openjdk.java.net/jdk/compare/ad83ec7e281cb3ab7c42e71fab47ea21b93079ea...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-04-04T20:01:09Z

@JoeWang-Java Pushed as commit 61d06c2.

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

8276050: XMLInputFactoryImpl.getProperty() returns null

7419095

openjdk bot added the rfr label Mar 31, 2022

openjdk bot added the core-libs label Mar 31, 2022

turbanoff reviewed Mar 31, 2022

View reviewed changes

reformat

c6b741c

RogerRiggs reviewed Apr 1, 2022

View reviewed changes

RogerRiggs approved these changes Apr 1, 2022

View reviewed changes

openjdk bot added the ready label Apr 1, 2022

openjdk bot added the integrated label Apr 4, 2022

openjdk bot closed this Apr 4, 2022

openjdk bot removed ready rfr labels Apr 4, 2022

JoeWang-Java deleted the JDK-8276050 branch April 5, 2022 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8276050: XMLInputFactoryImpl.getProperty() returns null #8063

8276050: XMLInputFactoryImpl.getProperty() returns null #8063

JoeWang-Java commented Mar 31, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 31, 2022

openjdk bot commented Mar 31, 2022

mlbridge bot commented Mar 31, 2022 •

edited

Loading

turbanoff Mar 31, 2022

JoeWang-Java Apr 1, 2022

RogerRiggs Apr 1, 2022

JoeWang-Java Apr 2, 2022

RogerRiggs left a comment

JoeWang-Java commented Apr 1, 2022

JoeWang-Java commented Apr 1, 2022

openjdk bot commented Apr 1, 2022 •

edited

Loading

JoeWang-Java commented Apr 4, 2022

openjdk bot commented Apr 4, 2022

openjdk bot commented Apr 4, 2022

8276050: XMLInputFactoryImpl.getProperty() returns null #8063

8276050: XMLInputFactoryImpl.getProperty() returns null #8063

Conversation

JoeWang-Java commented Mar 31, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 31, 2022

openjdk bot commented Mar 31, 2022

mlbridge bot commented Mar 31, 2022 • edited Loading

Webrevs

turbanoff Mar 31, 2022

Choose a reason for hiding this comment

JoeWang-Java Apr 1, 2022

Choose a reason for hiding this comment

RogerRiggs Apr 1, 2022

Choose a reason for hiding this comment

JoeWang-Java Apr 2, 2022

Choose a reason for hiding this comment

RogerRiggs left a comment

Choose a reason for hiding this comment

JoeWang-Java commented Apr 1, 2022

JoeWang-Java commented Apr 1, 2022

openjdk bot commented Apr 1, 2022 • edited Loading

JoeWang-Java commented Apr 4, 2022

openjdk bot commented Apr 4, 2022

openjdk bot commented Apr 4, 2022

JoeWang-Java commented Mar 31, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 31, 2022 •

edited

Loading

openjdk bot commented Apr 1, 2022 •

edited

Loading