8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() #7691

zhengyu123 · 2022-03-04T13:25:12Z

Please review this small patch that fixes a potential memory leak that exception return fails to release allocated cacheDirs

Test:

jdk_desktop on Linux x86_64

Progress

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

Issue

JDK-8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

Reviewers

Thomas Stuefe (@tstuefe - Reviewer)
David Holmes (@dholmes-ora - Reviewer)
Alexey Ivanov (@aivanov-jdk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7691

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

Using diff file

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

…onfig()

bridgekeeper · 2022-03-04T13:25:48Z

👋 Welcome back zgu! 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-04T13:28:50Z

@zhengyu123 The following labels will be automatically applied to this pull request:

client
core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-03-04T13:32:30Z

Webrevs

01: Full - Incremental (c4a3ec87)
00: Full (0ff07ddd)

tstuefe

Looks good.

openjdk · 2022-03-04T16:49:16Z

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

8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig()

Reviewed-by: stuefe, dholmes, aivanov

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

d07f7c7: 8282665: [REDO] ByteBufferTest.java: replace endless recursion with RuntimeException in void ck(double x, double y)
31ad80a: 8280902: ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
12693a6: 8282432: Optimize masked "test" Vector API with predicate feature
4924513: 8265263: AArch64: Combine vneg with right shift count
ea19114: 8282832: Update file path for HostnameMatcher/cert5.crt in test sun/security/util/Pem/encoding.sh
72e987e: 7192189: Support endpoint identification algorithm in RFC 6125
288d1af: 8282715: typo compileony in test Test8005033.java
6b34884: 8282234: Create a regression test for JDK-4532513
3fc009b: 8281560: Matcher.hitEnd returns unexpected results in presence of CANON_EQ flag.
2549e55: 8275640: (win) java.net.NetworkInterface issues with IPv6-only environments
... and 50 more: https://git.openjdk.java.net/jdk/compare/5c187e34a58769a129a0aae9e4937907c9060202...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.

prrace · 2022-03-04T19:37:10Z

/reviewers 3

openjdk · 2022-03-04T19:37:50Z

@prrace
The number of required reviews for this PR is now set to 3 (with at least 1 of role reviewers).

prrace · 2022-03-04T19:38:27Z

I've increased the number of reviewers because as well as the current reviewer it should have
(1) a reviewer from the client team
(2) a reviewer from whoever owns jni_util.h

If there were no change to that file it would have been simpler.

dholmes-ora

I would have just inlined JNU_CHECK_EXCEPTION and added the cleanup code directly. The additional macro wasn't really necessary and would not really be usable for any kind of general cleanup actions beyond a one-liner.

But it is okay.

Thanks.

zhengyu123 · 2022-03-04T21:54:57Z

I would have just inlined JNU_CHECK_EXCEPTION and added the cleanup code directly. The additional macro wasn't really necessary and would not really be usable for any kind of general cleanup actions beyond a one-liner.

But it is okay.

Thanks.

Thanks for the review, @dholmes-ora .

The macros are used to hide the different syntax of c and c++ to avoid polluting code here, it seems to be a common pattern in client code.

dholmes-ora · 2022-03-05T05:28:36Z

The macros are used to hide the different syntax of c and c++ to avoid polluting code here, it seems to be a common pattern in client code.

That's to allow for general use by C or C++ source files. But you are only needing this in a C source file so you would only need the C variant of the calling syntax.

mrserb · 2022-03-08T09:18:01Z

src/java.desktop/unix/native/common/awt/fontpath.c

@@ -935,7 +935,7 @@ Java_sun_font_FontConfigManager_getFontConfig
        if (cacheDirs != NULL) {
            while ((cnt < max) && (cacheDir = (*FcStrListNext)(cacheDirs))) {
                jstr = (*env)->NewStringUTF(env, (const char*)cacheDir);
-                JNU_CHECK_EXCEPTION(env);
+                JNU_CHECK_EXCEPTION_AND_CLEANUP(env, (*FcStrListDone)(cacheDirs));


I think you do not need to create an additional macro here, just inline it and call "(*FcStrListDone)(cacheDirs);" directly. Something like:

if (IS_NULL(jstr) { (*FcStrListDone)(cacheDirs); return; }

Note that the "IS_NULL" is used in this file after NewStringUTF. Any objections?

Good catch! Elsewhere in this file IS_NULL is always used to check NewStringUTF, so the existing code was inconsistent in checking for a pending exception.

Checking for NULL and checking for a pending exception are logically equivalent as long as the passed in pointer is not NULL.

mrserb · 2022-03-08T09:20:19Z

src/java.desktop/unix/native/common/awt/fontpath.c

@@ -935,7 +935,7 @@ Java_sun_font_FontConfigManager_getFontConfig
        if (cacheDirs != NULL) {
            while ((cnt < max) && (cacheDir = (*FcStrListNext)(cacheDirs))) {
                jstr = (*env)->NewStringUTF(env, (const char*)cacheDir);
-                JNU_CHECK_EXCEPTION(env);
+                JNU_CHECK_EXCEPTION_AND_CLEANUP(env, (*FcStrListDone)(cacheDirs));

                (*env)->SetObjectArrayElement(env, cacheDirArray, cnt++, jstr);


Probably we should add the check+cleanup after the SetObjectArrayElement? Otherwise, we may call NewStringUTF while an exception is raised by the SetObjectArrayElement.

??? You want to check and cleanup if NewStringUTF fails. You only want to call SetObjectElementArray if NewStringUTF succeeds.

Can SetObjectElementArray raise an exception?
The index is within the array length and we store a string. I assume cacheDirArray is a string array.

??? You want to check and cleanup if NewStringUTF fails. You only want to call SetObjectElementArray if NewStringUTF succeeds.

If the SetObjectArrayElement will throw an exception we will call the NewStringUTF while the exception is raised which is a kind of "jni" issue. If such an exception is possible we should check and cleanup.

A quick search for SetObjectArrayElement in java.desktop module shows the call to SetObjectArrayElement is rarely followed by exception check.

What kind of exception can SetObjectArrayElement raise if the index is within the range and the stored element is of correct type?

I think NewStringUTF() can throw OOM and also return NULL, just which one you pick.

I think NewStringUTF() can throw OOM and also return NULL, just which one you pick.

Yes.

But we're discussing whether a check for exception is necessary after (*env)->SetObjectArrayElement.

SetObjectArrayElement() may throw ArrayIndexOutOfBoundsException and ArrayStoreException, but I don't see it is possible here.

That's exactly my point. Therefore the check for exception after SetObjectArrayElement is unnecessary.

zhengyu123 · 2022-03-08T15:34:43Z

The macros are used to hide the different syntax of c and c++ to avoid polluting code here, it seems to be a common pattern in client code.

That's to allow for general use by C or C++ source files. But you are only needing this in a C source file so you would only need the C variant of the calling syntax.

I don't like the macros. I can argue all JNU_CHECK_EXCEPTION(_RETURN) macros are particular bad, because these patterns can easily result premature returns without proper cleanup. This bug is an evidence. But they exist for a long time, and I just follow the convention :-( I would prefer to eliminate them all, as inline code is not big at all.

I did a quick grep, there are a few suspicious spots, e.g. AwtWindow::SetIconData(), I have yet take a close look.

aivanov-jdk · 2022-03-08T15:58:57Z

I did a quick grep, there are a few suspicious spots, e.g. https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2711, I have yet take a close look.

It doesn't leak the icon handle here, it's assigned to the member of the object (probably NULL). Yet it's inconsistent: if an exception is expected from CreateIconFromRaster on the line above, why isn't it checked after CreateIconFromRaster is called on the following line?

zhengyu123 · 2022-03-08T17:48:39Z

I did a quick grep, there are a few suspicious spots, e.g. https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2711, I have yet take a close look.

It doesn't leak the icon handle here, it's assigned to the member of the object (probably NULL). Yet it's inconsistent: if an exception is expected from CreateIconFromRaster on the line above, why isn't it checked after CreateIconFromRaster is called on the following line?

I am not familiar with this code. What I see is that, it assigns m_hIcon and m_hIconSm to local variables, then reset them to NULL, once exception returns, are references to icon and small icon handles lost? and it will destroy icons as L2727 - 2732 do. But I may miss something.

aivanov-jdk · 2022-03-08T20:47:45Z

I did a quick grep, there are a few suspicious spots, e.g. https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/native/libawt/windows/awt_Window.cpp#L2711, I have yet take a close look.

It doesn't leak the icon handle here, it's assigned to the member of the object (probably NULL). Yet it's inconsistent: if an exception is expected from CreateIconFromRaster on the line above, why isn't it checked after CreateIconFromRaster is called on the following line?

I am not familiar with this code. What I see is that, it assigns m_hIcon and m_hIconSm to local variables, then reset them to NULL, once exception returns, are references to icon and small icon handles lost? and it will destroy icons as L2727 - 2732 do. But I may miss something.

You're right. The old icon handles in hOldIcon and hOldIconSm will be leaked here if CreateIconFromRaster throws an exception.

tstuefe

Still looks good to me.

aivanov-jdk · 2022-03-09T09:38:51Z

You're right. The old icon handles in hOldIcon and hOldIconSm will be leaked here if CreateIconFromRaster throws an exception.

I've submitted JDK-8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected

AlanBateman · 2022-03-09T10:05:41Z

/label remove core-libs

openjdk · 2022-03-09T10:07:33Z

@AlanBateman
The core-libs label was successfully removed.

dholmes-ora

Current version seems fine.

Thanks,
David

zhengyu123 · 2022-03-09T13:26:06Z

Thanks for the review and suggestions, @tstuefe, @dholmes-ora , @mrserb, @aivanov-jdk

zhengyu123 · 2022-03-09T13:26:14Z

/integrate

openjdk · 2022-03-09T13:27:21Z

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

d07f7c7: 8282665: [REDO] ByteBufferTest.java: replace endless recursion with RuntimeException in void ck(double x, double y)
31ad80a: 8280902: ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
12693a6: 8282432: Optimize masked "test" Vector API with predicate feature
4924513: 8265263: AArch64: Combine vneg with right shift count
ea19114: 8282832: Update file path for HostnameMatcher/cert5.crt in test sun/security/util/Pem/encoding.sh
72e987e: 7192189: Support endpoint identification algorithm in RFC 6125
288d1af: 8282715: typo compileony in test Test8005033.java
6b34884: 8282234: Create a regression test for JDK-4532513
3fc009b: 8281560: Matcher.hitEnd returns unexpected results in presence of CANON_EQ flag.
2549e55: 8275640: (win) java.net.NetworkInterface issues with IPv6-only environments
... and 50 more: https://git.openjdk.java.net/jdk/compare/5c187e34a58769a129a0aae9e4937907c9060202...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-03-09T13:27:38Z

@zhengyu123 Pushed as commit 5df2a05.

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

zhengyu123 · 2022-03-09T13:46:11Z

You're right. The old icon handles in hOldIcon and hOldIconSm will be leaked here if CreateIconFromRaster throws an exception.

I've submitted JDK-8282862: AwtWindow::SetIconData leaks old icon handles if an exception is detected

Thanks for the confirmation.

8282628: Potential memory leak in sun.font.FontConfigManager.getFontC…

0ff07dd

…onfig()

openjdk bot added the rfr Pull request is ready for review label Mar 4, 2022

openjdk bot added client client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Mar 4, 2022

tstuefe approved these changes Mar 4, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 4, 2022

openjdk bot removed the ready Pull request is ready to be integrated label Mar 4, 2022

dholmes-ora approved these changes Mar 4, 2022

View reviewed changes

mrserb reviewed Mar 8, 2022

View reviewed changes

mrserb and aivanov-jdk's comments

c4a3ec8

tstuefe approved these changes Mar 9, 2022

View reviewed changes

aivanov-jdk approved these changes Mar 9, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 9, 2022

openjdk bot removed the core-libs core-libs-dev@openjdk.org label Mar 9, 2022

dholmes-ora approved these changes Mar 9, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 9, 2022

openjdk bot closed this Mar 9, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 9, 2022

zhengyu123 deleted the JDK-8282628-leak-font-cfg branch March 9, 2022 18:24

8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() #7691

8282628: Potential memory leak in sun.font.FontConfigManager.getFontConfig() #7691

Conversation

zhengyu123 commented Mar 4, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 4, 2022

openjdk bot commented Mar 4, 2022

mlbridge bot commented Mar 4, 2022 • edited Loading

Webrevs

tstuefe left a comment

Choose a reason for hiding this comment

openjdk bot commented Mar 4, 2022 • edited Loading

prrace commented Mar 4, 2022

openjdk bot commented Mar 4, 2022

prrace commented Mar 4, 2022

dholmes-ora left a comment

Choose a reason for hiding this comment

zhengyu123 commented Mar 4, 2022

dholmes-ora commented Mar 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengyu123 commented Mar 8, 2022 • edited Loading

aivanov-jdk commented Mar 8, 2022 • edited Loading

zhengyu123 commented Mar 8, 2022

aivanov-jdk commented Mar 8, 2022

tstuefe left a comment

Choose a reason for hiding this comment

aivanov-jdk commented Mar 9, 2022

AlanBateman commented Mar 9, 2022

openjdk bot commented Mar 9, 2022

dholmes-ora left a comment

Choose a reason for hiding this comment

zhengyu123 commented Mar 9, 2022

zhengyu123 commented Mar 9, 2022

openjdk bot commented Mar 9, 2022

openjdk bot commented Mar 9, 2022

zhengyu123 commented Mar 9, 2022

zhengyu123 commented Mar 4, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 4, 2022 •

edited

Loading

openjdk bot commented Mar 4, 2022 •

edited

Loading

zhengyu123 commented Mar 8, 2022 •

edited

Loading

aivanov-jdk commented Mar 8, 2022 •

edited

Loading