8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected #22932

rajamah · 2025-01-06T17:38:24Z

Issue:
AwtWindow::SetIconData leaks the old icon handles in hOldIcon and hOldIconSm if CreateIconFromRaster raises an exception. Additionally, an exception is checked only after the first call to CreateIconFromRaster.

Solution:
I have added the exception handling code to take care that the handles are properly destroyed and not leaked.

Testing:
I have tested the code to make sure there are no regressions caused by this.

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-8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected (Bug - P3)

Reviewers

Harshitha Onkar (@honkar-jdk - Reviewer) Review applies to 505f804b
Alexey Ivanov (@aivanov-jdk - Reviewer)
Alexander Zvegintsev (@azvegint - Reviewer) Review applies to e22d8119
Dmitry Markov (@dmarkov20 - Reviewer)
Phil Race (@prrace - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22932

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…n is detected

bridgekeeper · 2025-01-06T17:39:04Z

👋 Welcome back rmahajan! 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 · 2025-01-06T17:39:51Z

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

8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected

Reviewed-by: aivanov, dmarkov, prr, honkar, 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 298 new commits pushed to the master branch:

356e2a8: 8348406: Remove tests GrantAllPermToExtWhenNoPolicy and PrincipalExpansionError from problem list
3ebf889: 8348327: Incorrect march flag when building libsleef/vector_math_neon.c
d9d2e19: 8348365: Bad format string in CLDRDisplayNamesTest
59e7509: 8348301: Remove unused Reference.waitForReferenceProcessing break-ins in tests
605b53e: 8348299: Update List/ItemEventTest/ItemEventTest.java
cba0f78: 8348387: Add fixpath if needed for user-supplied tools
44e5cca: 8348391: Keep case if possible for TOPDIR
7460a0a: 8348392: Make claims "other matches are possible" even when that is not true
5cc690d: 8347994: Add additional diagnostics to macOS failure handler to assist with diagnosing MCast test failures
c00557f: 8345049: Remove the jmx.tabular.data.hash.map compatibility property
... and 288 more: https://git.openjdk.org/jdk/compare/a77ed30fcc3360cd16a11b1899f52f7e871df1df...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@honkar-jdk, @aivanov-jdk, @azvegint, @dmarkov20, @prrace) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2025-01-06T17:40:21Z

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

client

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 · 2025-01-06T17:43:07Z

Webrevs

honkar-jdk

Please update the copyright year for .cpp file.
Is there a way to test this fix or possibly add a test case?

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

aivanov-jdk · 2025-01-07T20:39:56Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

+    try {
+        m_hIcon = CreateIconFromRaster(env, iconRaster, w, h);
+        JNU_CHECK_EXCEPTION(env); // Might throw here
+        m_hIconSm = CreateIconFromRaster(env, smallIconRaster, smw, smh);
+        JNU_CHECK_EXCEPTION(env); // Or here
+    } catch (...) {


At first I thought using C++ exception handling was a bad idea, but it's actually needed. The code in CreateIconFromRaster can throw a C++ exception, specifically BitmapUtil::CreateTransparencyMaskFromARGB and BitmapUtil::CreateV4BitmapFromARGB.

jdk/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

Lines 2075 to 2087 in e413fc6

try {

iconRasterBuffer = (int *)env->GetPrimitiveArrayCritical(iconRaster, 0);

JNI_CHECK_NULL_GOTO(iconRasterBuffer, "iconRaster data", done);

mask = BitmapUtil::CreateTransparencyMaskFromARGB(w, h, iconRasterBuffer);

image = BitmapUtil::CreateV4BitmapFromARGB(w, h, iconRasterBuffer);

} catch (...) {

if (iconRasterBuffer != NULL) {

env->ReleasePrimitiveArrayCritical(iconRaster, iconRasterBuffer, 0);

}

throw;

}

Since the exception is re-thrown in the handler there, it has to be caught in AwtWindow::SetIconData. Yet you shouldn't re-throw the C++ exception here, it must not escape JNI code, otherwise, the unhandled exception will likely crash the Java process.

aivanov-jdk · 2025-01-07T20:55:36Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

+
+    try {
+        m_hIcon = CreateIconFromRaster(env, iconRaster, w, h);
+        JNU_CHECK_EXCEPTION(env); // Might throw here


The comment is misleading, JNU_CHECK_EXCEPTION does not throw an exception.

jdk/src/java.base/share/native/libjava/jni_util.h

Lines 278 to 283 in 27646e5

#define JNU_CHECK_EXCEPTION(env) \

do { \

if ((env)->ExceptionCheck()) { \

return; \

} \

} while (0) \

It checks if an exception is raised in Java code and returns immediately if there's a pending Java exception.

If a Java exception is raised at this line, after m_hIcon is assigned and its value is not NULL, it has to be deleted.

Perhaps, you have to handle it explicitly here:

if (env->ExceptionCheck()) { if (m_hIcon) { DestroyIcon(m_hIcon); } return; }

It looks that the original values stored in hOldIcon and hOldIconSm should be restored if an error occurs.

aivanov-jdk · 2025-01-07T20:57:13Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

+        m_hIcon = CreateIconFromRaster(env, iconRaster, w, h);
+        JNU_CHECK_EXCEPTION(env); // Might throw here
+        m_hIconSm = CreateIconFromRaster(env, smallIconRaster, smw, smh);
+        JNU_CHECK_EXCEPTION(env); // Or here


The clean-up code here should delete both m_hIcon and m_hIconSm.

And restore the previous values?

aivanov-jdk · 2025-01-07T21:17:38Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

+        if (m_hIconSm != NULL) {
+            DestroyIcon(m_hIconSm);
+        }
+        throw; // Re-throw the exception


The C++ exception shouldn't be re-thrown here.

Whether we should raise a Java exception to indicate an error is to be discussed. If we restore the state of the AwtWindow object as if no failure occurred, it can continue. And I don't see any meaningful way to return an error back to Java here.

CreateIconFromRaster throws NullPointerException if it fails to get the image raster.

jdk/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

Lines 2076 to 2078 in e413fc6

iconRasterBuffer = (int *)env->GetPrimitiveArrayCritical(iconRaster, 0);

JNI_CHECK_NULL_GOTO(iconRasterBuffer, "iconRaster data", done);

where JNI_CHECK_NULL_GOTO is

jdk/src/java.desktop/windows/native/libawt/windows/awt.h

Lines 48 to 54 in 27646e5

#define JNI_CHECK_NULL_GOTO(obj, msg, where) { \

if (obj == NULL) { \

env->ExceptionClear(); \

JNU_ThrowNullPointerException(env, msg); \

goto where; \

} \

}

aivanov-jdk · 2025-01-10T20:42:19Z

The suggested fix does not address the reported issue yet: both hOldIcon and hOldIconSm are still leaked if the function exists prematurely.

Based on my above comments, I suggest restructuring the code.

Create new icons using CreateIconFromRaster. Do not assign the handles to member fields m_hIcon and m_hIconSm just yet.
Wrap the calls to CreateIconFromRaster in step 1 into a try-catch block to prevent a C++ exception escaping from JNI code as well as ensure a Java exception isn't raised.
1. If any exception occurs, destroy the newly created icon handles ((new_)hIcon and (new_)hIconSm) and exit.
  In this case both m_hIcon and m_hIconSm remain unmodified—the current icons remain unchanged.
2. If no exception occurs,
  1. Assign the old values of m_hIcon and m_hIconSm to temporary variables (hOldIcon and hOldIconSm),
    ~~or call DestroyIcon right on the member fields m_*~~;
  2. Assign the newly created icons to the member fields m_hIcon and m_hIconSm and execute the code below to update the inherited icons;
  3. Call DestroyIcon on hOldIcon and hOldIconSm.

aivanov-jdk

Looks good to me except for a tiny formatting nit.

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

aivanov-jdk · 2025-01-16T21:25:25Z

Is there a way to test this fix or possibly add a test case?

@honkar-jdk I don't think it's possible to write a test for this case….

It my be tested by defining OUTOFMEM_TEST in awt_new.cpp to enable random allocation failures.

Another way could be to change the source code to explicitly cause std::bad_alloc and/or a Java exception in any of the called methods. Just to validate the suggested fix really works… on top of static code analysis.

honkar-jdk · 2025-01-17T17:48:53Z

It my be tested by defining OUTOFMEM_TEST in awt_new.cpp to enable random allocation failures.

Another way could be to change the source code to explicitly cause std::bad_alloc and/or a Java exception in any of the called methods. Just to validate the suggested fix really works… on top of static code analysis.

@aivanov-jdk Thanks for clarifying.

…nction

prrace

Although this leak is extremely unlikely, leaks of GDI resources can bring even a modern windows system to its knees in a matter of seconds ..

prrace · 2025-01-17T22:40:09Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

 {
+    HICON hNewIcon = NULL;
+    HICON hNewIconSm = NULL;
+


I presume we know for sure there's no exception pending when we enter ?
Looking at the only caller, it seems probable.

Are you suggesting we add a JNU_CHECK_EXCEPTION the beginning of the function? , as I don't think we know for sure there is an exception pending or not at this point.

I presume we know for sure there's no exception pending when we enter ?
Looking at the only caller, it seems probable.

There can't be a pending exception when AwtWindow::SetIconData starts.

The chain starts with JNI method Java_sun_awt_windows_WWindowPeer_setIconImagesData which calls AwtWindow::_SetIconImagesData wrapped in SyncCall.

No Java code is called before execution gets into SetIconData where CreateIconFromRaster is called.

…I don't think we know for sure there is an exception pending or not at this point.

I'm sure there's no pending exception on the entry to AwtWindow::SetIconData.

prrace · 2025-01-17T22:42:09Z

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp

    HICON hOldIcon = NULL;
    HICON hOldIconSm = NULL;
-    //Destroy previous icon if it isn't inherited
    if ((m_hIcon != NULL) && !m_iconInherited) {


I am not sure why the check for NULL is needed .. but it is OK.

What do you mean? That it could've been just if ((m_hIcon) && !m_iconInherited)?

This line is unchanged.
In the majority of cases the surrounding code uses explicit != NULL in if-statements.

aivanov-jdk · 2025-01-20T15:56:09Z

While I was looking deeper into the code flow, I found another place which needs changing.

jdk/src/java.desktop/windows/native/libawt/windows/awt_Taskbar.cpp

Lines 126 to 132 in 0fbf10a

    
           JNIEXPORT void JNICALL Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon 
        
             (JNIEnv *env, jobject, jlong window, jintArray buf, jint w, jint h) 
        
           { 
        
               HICON icon = CreateIconFromRaster(env, buf, w, h); 
        
               m_Taskbar->SetOverlayIcon((HWND)window, icon, NULL); 
        
               ::DestroyIcon(icon); 
        
           }

The JNI method Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon for WTaskbarPeer.setOverlayIcon calls CreateIconFromRaster that can throw a C++ exception.

The call to CreateIconFromRaster has to be wrapped into a try-catch block to ensure the C++ exception does not escape.

I think it's reasonable to address this new issue under a new bugid.

aivanov-jdk · 2025-01-20T16:26:22Z

The call to CreateIconFromRaster has to be wrapped into a try-catch block to ensure the C++ exception does not escape.

I think it's reasonable to address this new issue under a new bugid.

I submitted JDK-8282862: Catch C++ exception in Java_sun_awt_windows_WTaskbarPeer_setOverlayIcon.

rajamah · 2025-01-23T20:21:01Z

/integrate

openjdk · 2025-01-23T20:22:26Z

@rajamah
Your change (at version 2f47a55) is now ready to be sponsored by a Committer.

aivanov-jdk · 2025-01-23T20:51:31Z

/sponsor

openjdk · 2025-01-23T20:52:47Z

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

356e2a8: 8348406: Remove tests GrantAllPermToExtWhenNoPolicy and PrincipalExpansionError from problem list
3ebf889: 8348327: Incorrect march flag when building libsleef/vector_math_neon.c
d9d2e19: 8348365: Bad format string in CLDRDisplayNamesTest
59e7509: 8348301: Remove unused Reference.waitForReferenceProcessing break-ins in tests
605b53e: 8348299: Update List/ItemEventTest/ItemEventTest.java
cba0f78: 8348387: Add fixpath if needed for user-supplied tools
44e5cca: 8348391: Keep case if possible for TOPDIR
7460a0a: 8348392: Make claims "other matches are possible" even when that is not true
5cc690d: 8347994: Add additional diagnostics to macOS failure handler to assist with diagnosing MCast test failures
c00557f: 8345049: Remove the jmx.tabular.data.hash.map compatibility property
... and 288 more: https://git.openjdk.org/jdk/compare/a77ed30fcc3360cd16a11b1899f52f7e871df1df...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-01-23T20:53:15Z

@aivanov-jdk @rajamah Pushed as commit 48ece07.

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

8282862: AwtWindow::SetIconData leaks old icon handles if an exceptio…

726fbd9

…n is detected

openjdk bot added the rfr Pull request is ready for review label Jan 6, 2025

openjdk bot added the client client-libs-dev@openjdk.org label Jan 6, 2025

fix indentation and make sure we delete both handles.

bdba3a9

honkar-jdk reviewed Jan 7, 2025

View reviewed changes

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp Outdated Show resolved Hide resolved

Update copyright year

79a82de

aivanov-jdk suggested changes Jan 7, 2025

View reviewed changes

Update code according to the feedback in code review

e22d811

aivanov-jdk approved these changes Jan 16, 2025

View reviewed changes

src/java.desktop/windows/native/libawt/windows/awt_Window.cpp Outdated Show resolved Hide resolved

openjdk bot added the ready Pull request is ready to be integrated label Jan 16, 2025

azvegint approved these changes Jan 17, 2025

View reviewed changes

dmarkov20 approved these changes Jan 17, 2025

View reviewed changes

leave opening bracre as is

505f804

honkar-jdk approved these changes Jan 17, 2025

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed ready Pull request is ready to be integrated labels Jan 17, 2025

check using not equal to NULL to match the rest of the code in the fu…

2f47a55

…nction

openjdk bot removed the ready Pull request is ready to be integrated label Jan 17, 2025

aivanov-jdk approved these changes Jan 17, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 17, 2025

prrace approved these changes Jan 17, 2025

View reviewed changes

dmarkov20 approved these changes Jan 22, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 23, 2025

openjdk bot added the integrated Pull request has been integrated label Jan 23, 2025

openjdk bot closed this Jan 23, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 23, 2025

	try {
	iconRasterBuffer = (int *)env->GetPrimitiveArrayCritical(iconRaster, 0);

	JNI_CHECK_NULL_GOTO(iconRasterBuffer, "iconRaster data", done);

	mask = BitmapUtil::CreateTransparencyMaskFromARGB(w, h, iconRasterBuffer);
	image = BitmapUtil::CreateV4BitmapFromARGB(w, h, iconRasterBuffer);
	} catch (...) {
	if (iconRasterBuffer != NULL) {
	env->ReleasePrimitiveArrayCritical(iconRaster, iconRasterBuffer, 0);
	}
	throw;
	}

	#define JNU_CHECK_EXCEPTION(env) \
	do { \
	if ((env)->ExceptionCheck()) { \
	return; \
	} \
	} while (0) \

	#define JNI_CHECK_NULL_GOTO(obj, msg, where) { \
	if (obj == NULL) { \
	env->ExceptionClear(); \
	JNU_ThrowNullPointerException(env, msg); \
	goto where; \
	} \
	}

8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected #22932

8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected #22932

Uh oh!

Conversation

rajamah commented Jan 6, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jan 6, 2025

Uh oh!

openjdk bot commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jan 6, 2025

Uh oh!

mlbridge bot commented Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

honkar-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk commented Jan 10, 2025

Uh oh!

aivanov-jdk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aivanov-jdk commented Jan 16, 2025

Uh oh!

honkar-jdk commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prrace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajamah Jan 17, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aivanov-jdk commented Jan 20, 2025

Uh oh!

aivanov-jdk commented Jan 20, 2025

Uh oh!

rajamah commented Jan 23, 2025

Uh oh!

openjdk bot commented Jan 23, 2025

Uh oh!

aivanov-jdk commented Jan 23, 2025

Uh oh!

openjdk bot commented Jan 23, 2025

Uh oh!

openjdk bot commented Jan 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

rajamah commented Jan 6, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 6, 2025 •

edited

Loading

mlbridge bot commented Jan 6, 2025 •

edited

Loading

honkar-jdk commented Jan 17, 2025 •

edited

Loading

rajamah Jan 17, 2025 •

edited

Loading