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

8049301: Suspicious use of string identity checks in JComponent.setUIProperty #4943

Closed
wants to merge 2 commits into from

Conversation

prsadhuk
Copy link
Contributor

@prsadhuk prsadhuk commented Jul 30, 2021

JComponent.setUIProperty method uses string identity check (==) rather than string equality checks (.equals) when comparing against the property name. This is suspicious since string identity and equality and equivalent only for interned strings.
Rectified to use String.equals() check.


Progress

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

Issue

  • JDK-8049301: Suspicious use of string identity checks in JComponent.setUIProperty

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4943

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 30, 2021

👋 Welcome back psadhukhan! 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 Pull request is ready for review label Jul 30, 2021
@openjdk
Copy link

openjdk bot commented Jul 30, 2021

@prsadhuk The following label will be automatically applied to this pull request:

  • swing

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 swing client-libs-dev@openjdk.org label Jul 30, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 30, 2021

Webrevs

@@ -4191,25 +4191,25 @@ void clientPropertyChanged(Object key, Object oldValue,
* @param value Object containing the property value
*/
void setUIProperty(String propertyName, Object value) {
if (propertyName == "opaque") {
if (propertyName.equals("opaque")) {
Copy link
Member

Choose a reason for hiding this comment

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

This will throw an NPE if propertyName is null, which didn't happened before the fix.

this method could be rewritten to use strings in switch.

Probably we might still want to use switch, it will handle the NPE case as before the fix.

Did you run CI tests after the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have taken care of NPE..I am not sure of switch which probably might be a hindrance in backporting if needed to earlier release trains.
CI is also run..link in JBS.

Copy link
Member

@azvegint azvegint Jul 30, 2021

Choose a reason for hiding this comment

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

I am not sure of switch which probably might be a hindrance in backporting if needed to earlier release trains.

You still can use the old switch statement with breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will not be consistent if we use switch only in this place because in most places, we either use the way we have before fix or or what I have done in version 1 or what I have done now. Probably, we need a PR to change other places where we have use pre-fix ways.

@openjdk
Copy link

openjdk bot commented Jul 30, 2021

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

8049301: Suspicious use of string identity checks in JComponent.setUIProperty

Reviewed-by: azvegint

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

  • 89f5c96: 8232066: Remove outdated code/methods from PKIX implementation
  • 9856ace: 8268963: [IR Framework] Some default regexes matching on PrintOptoAssembly in IRNode.java do not work on all platforms
  • b59418f: 8270060: (jdeprscan) tools/jdeprscan/tests/jdk/jdeprscan/TestRelease.java failed with class file for jdk.internal.util.random.RandomSupport not found
  • 4f42eb6: 8269523: runtime/Safepoint/TestAbortOnVMOperationTimeout.java failed when expecting 'VM operation took too long'
  • 77fbd99: 8270341: Test serviceability/dcmd/gc/HeapDumpAllTest.java timed-out
  • 048fb2c: Merge
  • 286d313: 8271489: (doc) Clarify Filter Factory example
  • d09b028: 8271396: Spelling errors
  • 489e5fd: 8268019: C2: assert(no_dead_loop) failed: dead loop detected
  • 6afcf5f: 8270886: Crash in PhaseIdealLoop::verify_strip_mined_scheduling
  • ... and 52 more: https://git.openjdk.java.net/jdk/compare/e4295ccfcdb16041d6f18fd64f7df3f740bf258f...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 Pull request is ready to be integrated label Jul 30, 2021
@prsadhuk
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jul 30, 2021

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

  • 89f5c96: 8232066: Remove outdated code/methods from PKIX implementation
  • 9856ace: 8268963: [IR Framework] Some default regexes matching on PrintOptoAssembly in IRNode.java do not work on all platforms
  • b59418f: 8270060: (jdeprscan) tools/jdeprscan/tests/jdk/jdeprscan/TestRelease.java failed with class file for jdk.internal.util.random.RandomSupport not found
  • 4f42eb6: 8269523: runtime/Safepoint/TestAbortOnVMOperationTimeout.java failed when expecting 'VM operation took too long'
  • 77fbd99: 8270341: Test serviceability/dcmd/gc/HeapDumpAllTest.java timed-out
  • 048fb2c: Merge
  • 286d313: 8271489: (doc) Clarify Filter Factory example
  • d09b028: 8271396: Spelling errors
  • 489e5fd: 8268019: C2: assert(no_dead_loop) failed: dead loop detected
  • 6afcf5f: 8270886: Crash in PhaseIdealLoop::verify_strip_mined_scheduling
  • ... and 52 more: https://git.openjdk.java.net/jdk/compare/e4295ccfcdb16041d6f18fd64f7df3f740bf258f...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 30, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 30, 2021
@openjdk
Copy link

openjdk bot commented Jul 30, 2021

@prsadhuk Pushed as commit baf7797.

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

@prsadhuk prsadhuk deleted the 8049301 branch July 30, 2021 15:40
@aivanov-jdk
Copy link
Member

I believe it was an optimisation to make comparison as quick as possible. There are many places in Swing where property keys are compared as object identity rather than using equals. If this place is changed, probably all other such places need updating if they haven't been updated to equals yet.

@mrserb
Copy link
Member

mrserb commented Jul 30, 2021

FYI the "==" was used intentionally for the performance reason and it worked fine since the property's names are interned(used as string literals only).

@mrserb
Copy link
Member

mrserb commented Jul 30, 2021

I believe it was an optimisation to make comparison as quick as possible. There are many places in Swing where property keys are compared as object identity rather than using equals. If this place is changed, probably all other such places need updating if they haven't been updated to equals yet.

Probably it will be better to roll back this one, and carefully check the usage of "==" for possible replacement by the "equal". That possibility should be proved by some test cases.

@aivanov-jdk
Copy link
Member

I believe it was an optimisation to make comparison as quick as possible. There are many places in Swing where property keys are compared as object identity rather than using equals. If this place is changed, probably all other such places need updating if they haven't been updated to equals yet.

Probably it will be better to roll back this one, and carefully check the usage of "==" for possible replacement by the "equal". That possibility should be proved by some test cases.

It's the other way around: equals can always be used (but could be slower); however, == may produce incorrect results in some cases. Probably, such optimisation == vs equals doesn't make sense any more and we should replace all usages of == with equals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated swing client-libs-dev@openjdk.org
4 participants