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

[Andrew John Hughes]

On Mon, 11 Apr 2022 18:05:09 GMT, Zhengyu Gu <zgu at openjdk.org> wrote:

> Please review this small patch that releases temporary charsets to avoid memory leak.
> 
> Test:
> 
> - [x] jdk_2d

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;

-------------

PR: https://git.openjdk.java.net/jdk/pull/8187