8284680: sun.font.FontConfigManager.getFontConfig() leaks charset #8187

zhengyu123 · 2022-04-11T18:05:09Z

Please review this small patch that releases temporary charsets to avoid memory leak.

Test:

jdk_2d

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 reviewer, 1 author)

Issue

JDK-8284680: sun.font.FontConfigManager.getFontConfig() leaks charset

Reviewers

Phil Race (@prrace - Reviewer)
Andrew John Hughes (@gnu-andrew - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8187

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

Using diff file

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

bridgekeeper · 2022-04-11T18:06:03Z

👋 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-04-11T18:07:59Z

@zhengyu123 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 · 2022-04-11T18:12:06Z

Webrevs

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

prrace · 2022-04-27T18:02:56Z

/reviewers 2

openjdk · 2022-04-27T18:04:47Z

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

8284680: sun.font.FontConfigManager.getFontConfig() leaks charset

Reviewed-by: prr, andrew

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

cec23b1: 8286556: Remove EagerInitialization develop option
89de756: 8286387: Remove unused FreeListAllocator::reduce_free_list
ae695d6: 8286476: x86_32: Fix crashes with non-preview mode after JDK-8284161 (Virtual Threads)
87f3d2b: 8286446: PPC64: fix crashes after JDK-8284161 (virtual threads preview)
7a2bbbb: 8286396: Address possibly lossy conversions in jdk.management.jfr
9ac52b0: 8286392: Address possibly lossy conversions in jdk.jfr
f628966: 8286541: JFR: RecordingFile.write is missing "since 19"
f1554fc: 8285872: JFR: Remove finalize() methods
faa1aad: 8286515: JFR: Remove SimpleStringIdPool class
7612bba: 8285698: Create a test to check the focus stealing of JPopupMenu from JComboBox
... and 413 more: https://git.openjdk.java.net/jdk/compare/92f5e42696847de7e47e238412d2d541a11bccd7...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 · 2022-04-27T18:05:12Z

@prrace
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with 1 of role reviewers, 1 of role authors).

gnu-andrew

Ok, let me see if I follow this correctly.

The main logic to call FcCharSetDestroy is within two loops. The outermost loop declares unionCharset. The inner loop declares charset.

On each of the iterations of the inner loop, charset is newly defined and obtains a character set via FcPatternGetString. This character set is not a copy and doesn't need to be freed.

I thus assume the need to free comes from the return value of FcCharSetUnion. On each iteration of the inner loop, this is used to add more characters to the character set pointed to by unionCharset.

On iteration 0, unionCharset is NULL so it is assigned charset, which is the character set for fontset->fonts[0].
On iteration 1, unionCharset is now the charset for fontset->fonts[0]. charset will be redefined and set to the character set for fontset->fonts[1].
I see a problem on this iteration. currentUnionSet will be set to the character set for fontset->fonts[0] and unionCharset will then be allocated a new character set consisting of the union of the characters in the sets for fontset->fonts[0] (in currentUnionSet) and fontset->fonts[1] (in charset). How will currentUnionSet ever be equal to charset in this case?

Unless I'm missing something, the second iteration is going to wrongly attempt to free the character set allocated in the first iteration.

For subsequent iterations, the free is fine, because it is indeed releasing the previous union.

A possible solution would be to introduce another variable e.g. previousUnion which is only set after the first union is created. The problem with using unionCharset is it is set to charset on the first iteration.

So something like:

unionCharset = (* FcCharSetUnion)(unionCharset, charset);
if (previousUnion != NULL) {
  (*FcCharSetDestroy)(previousUnion);
}
previousUnion = unionCharset;

zhengyu123 · 2022-04-28T20:12:52Z

Hmmm, you are right. Phil probably pointed out the same problem, but I misunderstood it.

What's odd is that, I tested (made sure that FcCharSetDestroy indeed called), it did not crash and valgrind showed the leak site disappeared.

With your suggested fix, I think we still leak last unionCharset. BTW, the API documentation is really unhelpful, Sigh!

gnu-andrew · 2022-05-04T01:39:49Z

Hmmm, you are right. Phil probably pointed out the same problem, but I misunderstood it.

What's odd is that, I tested (made sure that FcCharSetDestroy indeed called), it did not crash and valgrind showed the leak site disappeared.

With your suggested fix, I think we still leak last unionCharset. BTW, the API documentation is really unhelpful, Sigh!

That's fixed by your addition at the end, right?

I assumed the final unionCharset was used for something, but it never seems to actually be read, just continually added to and then drops out of scope.

It may also need to be freed in the if (result != FcResultMatch) { block which frees everything else and returns from within the inner loop.

zhengyu123 · 2022-05-09T19:41:34Z

Hmmm, you are right. Phil probably pointed out the same problem, but I misunderstood it.
What's odd is that, I tested (made sure that FcCharSetDestroy indeed called), it did not crash and valgrind showed the leak site disappeared.
With your suggested fix, I think we still leak last unionCharset. BTW, the API documentation is really unhelpful, Sigh!

That's fixed by your addition at the end, right?

I assumed the final unionCharset was used for something, but it never seems to actually be read, just continually added to and then drops out of scope.

if ((*FcCharSetSubtractCount)(charset, unionCharset) > minGlyphs) {
Seems this is the only use of unionCharset.

It may also need to be freed in the if (result != FcResultMatch) { block which frees everything else and returns from within the inner loop.

Right, updated.

gnu-andrew

Looks good to me. Hopefully we caught everything.

zhengyu123 · 2022-05-12T12:24:22Z

Looks good to me. Hopefully we caught everything.

Thanks @gnu-andrew.

zhengyu123 · 2022-05-12T12:24:45Z

Thanks @prrace for the review.

zhengyu123 · 2022-05-12T12:24:59Z

/integrate

openjdk · 2022-05-12T12:26:37Z

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

40f43c6: 8286601: Mac Aarch: Excessive warnings to be ignored for build jdk
be97b4b: 8278348: [macos12] javax/swing/JTree/4908142/bug4908142.java fails in macos12
ff17f49: 8284888: [macos] javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java failed with "NimbusLookAndFeel] : ERROR: icon and imageIcon not same."
50d47de: 8286582: Build fails on macos aarch64 when using --with-zlib=bundled
89392fb: 8285820: C2: LCM prioritizes locally dependent CreateEx nodes over projections after 8270090
96d48f3: 8286433: Cache certificates decoded from TLS session tickets
7567627: 8286467: G1: Collection set pruning adds one region too many
82d2570: 8283001: windows-x86-cmp-baseline fails in some jvmti native libs
e9f45bb: 8282966: AArch64: Optimize VectorMask.toLong with SVE2
57a7670: 8285612: Remove jtreg tag manual=yesno for java/awt/print/PrinterJob/ImagePrinting/ClippedImages.java
... and 430 more: https://git.openjdk.java.net/jdk/compare/92f5e42696847de7e47e238412d2d541a11bccd7...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-12T12:26:49Z

@zhengyu123 Pushed as commit dea6e88.

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

v0

ce50c4e

openjdk bot added the rfr Pull request is ready for review label Apr 11, 2022

openjdk bot added the client client-libs-dev@openjdk.org label Apr 11, 2022

prrace reviewed Apr 15, 2022

View reviewed changes

src/java.desktop/unix/native/common/awt/fontpath.c Show resolved Hide resolved

prrace approved these changes Apr 27, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 27, 2022

openjdk bot removed the ready Pull request is ready to be integrated label Apr 27, 2022

gnu-andrew reviewed Apr 28, 2022

View reviewed changes

Andrew's comment

92ed6a7

prrace approved these changes May 9, 2022

View reviewed changes

Cleanup at early return

2c74ef9

prrace approved these changes May 9, 2022

View reviewed changes

gnu-andrew approved these changes May 11, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 11, 2022

openjdk bot added the integrated Pull request has been integrated label May 12, 2022

openjdk bot closed this May 12, 2022

openjdk bot removed the ready Pull request is ready to be integrated label May 12, 2022

openjdk bot removed the rfr Pull request is ready for review label May 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8284680: sun.font.FontConfigManager.getFontConfig() leaks charset #8187

8284680: sun.font.FontConfigManager.getFontConfig() leaks charset #8187

zhengyu123 commented Apr 11, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 11, 2022

openjdk bot commented Apr 11, 2022

mlbridge bot commented Apr 11, 2022 •

edited

Loading

prrace commented Apr 27, 2022

openjdk bot commented Apr 27, 2022 •

edited

Loading

openjdk bot commented Apr 27, 2022

gnu-andrew left a comment •

edited

Loading

zhengyu123 commented Apr 28, 2022

gnu-andrew commented May 4, 2022

zhengyu123 commented May 9, 2022

gnu-andrew left a comment

zhengyu123 commented May 12, 2022

zhengyu123 commented May 12, 2022

zhengyu123 commented May 12, 2022

openjdk bot commented May 12, 2022

openjdk bot commented May 12, 2022

8284680: sun.font.FontConfigManager.getFontConfig() leaks charset #8187

8284680: sun.font.FontConfigManager.getFontConfig() leaks charset #8187

Conversation

zhengyu123 commented Apr 11, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 11, 2022

openjdk bot commented Apr 11, 2022

mlbridge bot commented Apr 11, 2022 • edited Loading

Webrevs

prrace commented Apr 27, 2022

openjdk bot commented Apr 27, 2022 • edited Loading

openjdk bot commented Apr 27, 2022

gnu-andrew left a comment • edited Loading

Choose a reason for hiding this comment

zhengyu123 commented Apr 28, 2022

gnu-andrew commented May 4, 2022

zhengyu123 commented May 9, 2022

gnu-andrew left a comment

Choose a reason for hiding this comment

zhengyu123 commented May 12, 2022

zhengyu123 commented May 12, 2022

zhengyu123 commented May 12, 2022

openjdk bot commented May 12, 2022

openjdk bot commented May 12, 2022

zhengyu123 commented Apr 11, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 11, 2022 •

edited

Loading

openjdk bot commented Apr 27, 2022 •

edited

Loading

gnu-andrew left a comment •

edited

Loading