8341792: Fix ExceptionOccurred in java.security.jgss #21424

wangweij · 2024-10-09T16:22:13Z

Switch to ExceptionCheck.

This is a part of an umbrella bug JDK-8341542 JNI uses of ExceptionOccurred() treated as if function returns a bool.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8341792: Fix ExceptionOccurred in java.security.jgss (Sub-task - P4)

Reviewers

Justin Lu (@justin-curtis-lu - Committer)
Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21424/head:pull/21424
$ git checkout pull/21424

Update a local copy of the PR:
$ git checkout pull/21424
$ git pull https://git.openjdk.org/jdk.git pull/21424/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21424

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21424.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-09T16:23:14Z

👋 Welcome back weijun! 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 · 2024-10-09T16:24:44Z

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

8341792: Fix ExceptionOccurred in java.security.jgss

Reviewed-by: jlu, mullan

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

1604255: 8341619: C2: remove unused StoreCM node
e7c5bf4: 8341722: Fix some warnings as errors when building on Linux with toolchain clang
36fca5d: 8341882: [BACKOUT] java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
780de00: 8051591: Test javax/swing/JTabbedPane/8007563/Test8007563.java fails
313f4a9: 8340809: Open source few more AWT PopupMenu tests
9d621d3: 8338884: java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
475f8f9: 8341859: Optimize ClassFile Benchmark Write
c850ecb: 8341755: Optimize argNames in InnerClassLambdaMetafactory
172f744: 8340985: Open source some Desktop related tests
e7045e9: 8341684: Typo in External Specifications link of java.util.Currency
... and 60 more: https://git.openjdk.org/jdk/compare/b42fbf43dfd62ae64973ff0e406b6609cd8e1aa6...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 · 2024-10-09T16:32:14Z

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

security

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 · 2024-10-09T16:35:29Z

Webrevs

00: Full (2a6a5de7)

justin-curtis-lu · 2024-10-09T17:23:08Z

src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m

@@ -51,7 +51,7 @@ void _SCDynamicStoreCallBack(SCDynamicStoreRef store, CFArrayRef changedKeys, vo
        jmethodID jm_Config_refresh = (*env)->GetStaticMethodID(env, jc_Config, "refresh", "()V");
        CHECK_NULL(jm_Config_refresh);
        (*env)->CallStaticVoidMethod(env, jc_Config, jm_Config_refresh);
-        if ((*env)->ExceptionOccurred(env) != NULL) {


I believe we can keep the original behavior if you want to, because its being compared to NULL and not as a boolean.

Returns the exception object that is currently in the process of being thrown, or NULL if no exception is currently being thrown.

But the new code might read better. So I guess it is just preference.

One concern pointed out by the umbrella bug is that ExceptionOccured(env) returns a jthrowable reference that should be freed by DeleteLocalRef(jthrowable). Even if the reference could be automatically freed later, it's probably still a good practice to either free them early or just not create them at all.

mcpowers · 2024-10-10T14:08:05Z

Looks good to me.

artur-oracle · 2024-10-10T14:22:26Z

src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m

Copyright year is not updated

wangweij · 2024-10-10T17:21:51Z

/integrate

openjdk · 2024-10-10T17:22:48Z

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

6fad6af: 8341819: LightweightSynchronizer::enter_for races with deflation
4beb771: 8339538: Wrong timeout computations in DnsClient
a7d2077: 8340366: Open source several AWT Dialog tests - Batch 3
2d8fcc4: 8340193: Open source several AWT Dialog tests - Batch 1
6e01384: 8341136: Optimize StackMapGenerator::trimAndCompress
e9327b6: 8339205: Optimize StackMapGenerator
1604255: 8341619: C2: remove unused StoreCM node
e7c5bf4: 8341722: Fix some warnings as errors when building on Linux with toolchain clang
36fca5d: 8341882: [BACKOUT] java/nio/file/attribute/BasicFileAttributeView/CreationTime.java#tmp fails on alinux3
780de00: 8051591: Test javax/swing/JTabbedPane/8007563/Test8007563.java fails
... and 66 more: https://git.openjdk.org/jdk/compare/b42fbf43dfd62ae64973ff0e406b6609cd8e1aa6...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-10T17:22:55Z

@wangweij Pushed as commit 76541b0.

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

the fix

2a6a5de

openjdk bot added the rfr Pull request is ready for review label Oct 9, 2024

openjdk bot added the security security-dev@openjdk.org label Oct 9, 2024

wangweij changed the title ~~8341792: Fix locations in java.security.jgss~~ 8341792: Fix ExceptionOccurred in java.security.jgss Oct 9, 2024

justin-curtis-lu reviewed Oct 9, 2024

View reviewed changes

justin-curtis-lu approved these changes Oct 9, 2024

View reviewed changes

seanjmullan approved these changes Oct 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 10, 2024

artur-oracle reviewed Oct 10, 2024

View reviewed changes

src/java.security.jgss/macosx/native/libosxkrb5/SCDynamicStoreConfig.m

Copy link

Member

artur-oracle Oct 10, 2024

Choose a reason for hiding this comment

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

Copyright year is not updated

openjdk bot added the integrated Pull request has been integrated label Oct 10, 2024

openjdk bot closed this Oct 10, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 10, 2024

wangweij deleted the 8341792 branch December 5, 2024 02:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

8341792: Fix ExceptionOccurred in java.security.jgss #21424

8341792: Fix ExceptionOccurred in java.security.jgss #21424

Uh oh!

wangweij commented Oct 9, 2024 •

edited by openjdk bot

Loading

Uh oh!

bridgekeeper bot commented Oct 9, 2024

Uh oh!

openjdk bot commented Oct 9, 2024 •

edited

Loading

Uh oh!

openjdk bot commented Oct 9, 2024

Uh oh!

mlbridge bot commented Oct 9, 2024

Uh oh!

justin-curtis-lu Oct 9, 2024 •

edited

Loading

Uh oh!

wangweij Oct 9, 2024

Uh oh!

mcpowers commented Oct 10, 2024

Uh oh!

artur-oracle Oct 10, 2024

Uh oh!

wangweij commented Oct 10, 2024

Uh oh!

openjdk bot commented Oct 10, 2024

Uh oh!

openjdk bot commented Oct 10, 2024

Uh oh!

Uh oh!

8341792: Fix ExceptionOccurred in java.security.jgss #21424

8341792: Fix ExceptionOccurred in java.security.jgss #21424

Uh oh!

Conversation

wangweij commented Oct 9, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 9, 2024

Uh oh!

openjdk bot commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 9, 2024

Uh oh!

mlbridge bot commented Oct 9, 2024

Webrevs

Uh oh!

justin-curtis-lu Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wangweij Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

mcpowers commented Oct 10, 2024

Uh oh!

artur-oracle Oct 10, 2024

Choose a reason for hiding this comment

Uh oh!

wangweij commented Oct 10, 2024

Uh oh!

openjdk bot commented Oct 10, 2024

Uh oh!

openjdk bot commented Oct 10, 2024

Uh oh!

Uh oh!

wangweij commented Oct 9, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 9, 2024 •

edited

Loading

justin-curtis-lu Oct 9, 2024 •

edited

Loading