Memory leak in ass_font_provider_add_font

[clsid2]

Memory leak in ass_font_provider_add_font

[BigDaddy-HotSpot]

So, because I'm new to the programing and coding world, I am still unfamiliar with all the abbreviated commands and word syntax. I did get a good laugh though reading along. If I didn't know better, I would have thought I was in an OFs sub-Reddit with all the mentions of leaked ass, freed ass, and implicit family members. So, thanks for that. 👍

[clsid2]

Leak shows up when closing the player. Libass is fully deinitialised. It leaks data for each non-embedded system font that is loaded. No DirectWrite. Git head for libass and also up-to-date other libs.

Here is example of leak details with three fonts loaded:
https://pastebin.com/H5XuYKtV

Adding this before the return true fixes the leak. Would of course need a pointer check as well before freeing.

free_font_info(&implicit_meta);
free(implicit_meta.postscript_name);

[rcombs]

As long as the font has any family names (which should always be the case), the leaking code path doesn't run when using fontconfig, memory fonts, or sufficiently-new windows. It's exclusive to macOS and older win32.

[TheOneric]

Thanks for the report and initial analysis!
Can you give some more info on which fields leak and how you are using the API? Does the leak manifest in memory being leftover after the library has been fully deinitialised / MPC-HC shutdown, or in increasing memory usage while the MPC-HC is running (but everything is properly freed during deinit/shutdown)? Does it happen with the DirectWrite backend (Desktop or UWP variant? see libass logs), Fontconfig or both?
Also, which libass commit is MPC-HC using?

Digging through the implementation and all changes to MPC-HC’s libass backend is a bit much.

[clsid2]

The issue might be specific to certain Windows versions. A fellow dev could reproduce it on Win7, but not on Win10.

This code segment also seems wrong to me:

libass/libass/ass_fontselect.c

Lines 429 to 436 in d1f0f20

	if (implicit_meta.postscript_name) {
	implicit_meta.postscript_name =
	strdup(implicit_meta.postscript_name);
	if (!implicit_meta.postscript_name) {
	FT_Done_Face(face);
	goto error;
	}
	}

One of these implicit_meta probably should be meta?

This also related to issue I think:

libass/libass/ass_fontselect.c

Line 439 in d1f0f20

meta = &implicit_meta;

Copy all data to the struct that you keep and destroy the other one. Don't assign one to another.

[adipose]

The block is attempting to initialize a new meta which it assigns to implicit_meta at the end. I guess get_font_info fills up implicit_meta. Maybe code should be:

        if ( !implicit_meta.postscript_name) {
            implicit_meta.postscript_name =
                strdup(meta->postscript_name);

In other words, if get_font_info doesn't get a postscript_name, we'll use the one from the meta we are about to discard.

Well, that doesn't work, since meta seems to have nothing initialized, either.

[adipose]

By the way, in Win10 it doesn't get into ass_font_provider_add_font, it seems.

Edit: it does, but it doesn't get into the block if (!meta->n_family)

In Win7, it does get in there, because IDWriteFontFace_QueryInterface fails earlier with HR=E_NOTIMPL.

https://learn.microsoft.com/en-us/windows/win32/api/dwrite_3/nn-dwrite_3-idwritefontface3

Minimum supported client | Windows 10 [desktop apps | UWP apps]

At least this explains why Win7 has the memory leak and Win10 doesn't.

[adipose]

//implicit_meta.postscript_name = strdup(implicit_meta.postscript_name);

Just removing this code seems to eliminate the leak, and it didn't seem to have any negative impact on the font rendering. On line 514, later:

info->postscript_name = strdup(meta->postscript_name);

It gets copied again. I debugged this and it there's never an issue with the buffer being destroyed before here, at which point, it shouldn't be needed again.

It gets freed here (ass_directwrite.c) line 645:

free(meta.postscript_name)

[rcombs]

Just removing this code seems to eliminate the leak

It also adds a use-after-free; the FT_Done_Face(face); frees the original value of implicit_meta.postscript_name (which is why the strdup is there in the first place).

I can repro this on macOS, and I see it also leaks implicit_meta.families, implicit_meta.fullnames, and their respective content strings (which makes sense looking over the code); obviously skipping a copy here doesn't solve that either.

Adding free_font_info(&implicit_meta); and free(implicit_meta.postscript_name); at the end seems sensible to me. This looks like a regression in 887e6cc; @astiob any idea what happened there, or if there's any reason not to make the proposed change?

We should probably add some basic leak tests on the CI. On macOS, this is trivial:

leaks --atExit -- ./test/test out.png tests/crash/generic.ass 2

^ This returns 1 with the current code (and dumps a lot of diagnostic information), and 0 with the proposed fix. Are there similar tools available on Linux and Windows? If not, I think it would still be valuable even if it's macOS-only.

[clsid2]

For me meta just contains null values when doing the cleanup. But weird thing is that it contains valid values just before returning true in ass_font_provider_add_font.

libass/libass/ass_directwrite.c

Lines 630 to 633 in d1f0f20

	ass_font_provider_add_font(provider, &meta, NULL, 0, font_priv);
	cleanup:
	if (meta.families) {

[clsid2]

This seems to fix the problem:
meta = &implicit_meta; -> *meta = implicit_meta;

[rcombs]

That's meta (in the caller), not implicit_meta (within ass_font_provider_add_font). When the passed-in meta has a nonzero n_family, the call to get_font_info isn't made, and none of the leaking allocations occur in the first place. Whether or not this occurs can indeed depend on the Windows version.

[rcombs]

This seems to fix the problem:
meta = &implicit_meta; -> *meta = implicit_meta;

Sure, and it also introduces a leak if the original meta contained anything valid (for instance, a postscript name, as used on CoreText).

[clsid2]

Then my original proposed fix is the way to go. That solves the leak here.

free_font_info(&implicit_meta);
free(implicit_meta.postscript_name);
return true;

[adipose]

Then my original proposed fix is the way to go. That solves the leak here.

free_font_info(&implicit_meta);
free(implicit_meta.postscript_name);
return true;

Knowing that it will be a dup, I suppose you could change

info->postscript_name = strdup(meta->postscript_name);

to info->postscript_name = meta->postscript_name;

It's already been copied so you don't need to do it again?

[TheOneric]

We should probably add some basic leak tests on the CI. On macOS, this is trivial:

We already compile with ASAN for glibc-Linux and MacOS in CI and on Linux ASAN’s leak detection is enabled by default. Either this problem doesn't occur with our testcases and we should add more testcases, or it just doesn't occur with Fontconfig in general.

leaks --atExit -- ./test/test out.png tests/crash/generic.ass 2

^ This returns 1 with the current code (and dumps a lot of diagnostic information), and 0 with the proposed fix.

Wasn't there also an ASAN env variable to enable leak detection on MacOS? If ASAN’s leak detection works about as well as this leak tool this would be the easiest adjustment to get the detection for MacOS CI.
If not, is this tool compatible with ASAN, or will we need two MacOS runners or drop ASAN for leak detection?

[adipose]

This is happening with directwrite (windows only) and only when font write api is less than 3 (Windows 10).

[rcombs]

Looks like asan's leak detection doesn't work in apple clang (it just gives an error about the platform being supported), and leaks doesn't seem to be compatible with asan builds. I can use ASAN_OPTIONS=detect_leaks=1 with an asan build made with homebrew clang, but it gives garbage output (seemingly confused by stuff going on within CoreText and system runtime code).

[TheOneric]

This is happening with directwrite (windows only) and only when font write api is less than 3 (Windows 10).

yeah, thanks for nailing it down like that!

Apparently though it also affects CoreText as rcombs was able to reproduce leaks with it on MacOS. But we haven't seen any leaks yet in our CI, which atm only checks for leaks with Fontconfig as the system font provider. So the questions are:

• are our CI testcases insufficient to trigger the leak and Fontconfig is also affected, or is Fontconfig wholly unaffected (might become relevant for how to properly fix this)
• how to best get leak detection in CI for other platforms/fontproviders to catch such bugs earlier in the future

Though the Windows systems available in GHA should be new enough to already include Windows 10 APIs, so we wouldn't have automatically caught the DirectWrite leak either way (but catching the CoreText leak might have also led to it being discovered and/or fixed alongside).

[TheOneric]

Welp, ASAN_OPTIONS=detect_leaks=1 not working and leaks being incompatible with ASAN is unfortunate. Guess we’ll have to add a ninth (tenth with the TSAN build from the threading PR) CI job (for as long as GHA doesn't start to impose limits).

[adipose]

Just removing this code seems to eliminate the leak

It also adds a use-after-free; the FT_Done_Face(face); frees the original value of implicit_meta.postscript_name (which is why the strdup is there in the first place).

I can repro this on macOS, and I see it also leaks implicit_meta.families, implicit_meta.fullnames, and their respective content strings (which makes sense looking over the code); obviously skipping a copy here doesn't solve that either.

I don't know why these additional leaks occur, but it doesn't seem to happen in the scenario clsid2 identified on Windows. This leak seems to be specifically due to copying the buffer before FT_Done_Face would free it. The rest of the dups below are called more generally, right? And this buffer gets copied again along with everything else.

[clsid2]

Actually those do leak.

The indentified leaks are the allocations in
ass_fontselect.c
Lines 287/329/337/431

[adipose]

This seems to fix the problem:

meta = &implicit_meta; -> *meta = implicit_meta;

Maybe if this function passed **meta instead...pointing meta to the new struct doesn't permit the calling routine to free it as the pointer is passed by value. When it returns, meta is back to its original value.

[clsid2]

I think that is intentional, so that the original meta can be freed. But it does mean that implicit_meta must be freed within the function.

[adipose]

I think that is intentional, so that the original meta can be freed. But it does mean that implicit_meta must be freed within the function.

Agreed. Which means yours is the only fix. If a new "meta" has been created it has to be freed here.

[adipose]

An observation--the below code is redundant, perhaps could be consolidated. Perhaps a cleaner approach would be to initialize meta (get_font_info_IDWriteFontFace3(face3, &meta) for example), or (if that fails), call a separate function to initialize meta (using the if-block code from add_font_face). Once meta is successfully initialized, add_font_face can be called with a reliable meta. Then a common call free_font_info can be used on whatever meta is initialized.

From directwrite.c:

    if (meta.families) {
        for (int k = 0; k < meta.n_family; k++)
            free(meta.families[k]);
        free(meta.families);
    }

    if (meta.fullnames) {
        for (int k = 0; k < meta.n_fullname; k++)
            free(meta.fullnames[k]);
        free(meta.fullnames);
    }

    free(meta.postscript_name);

From ass_fontselect.c

    free_font_info(&implicit_meta);
    free(implicit_meta.postscript_name);
...
static void free_font_info(ASS_FontProviderMetaData *meta)
{
    if (meta->families) {
        for (int i = 0; i < meta->n_family; i++)
            free(meta->families[i]);
        free(meta->families);
    }

    if (meta->fullnames) {
        for (int i = 0; i < meta->n_fullname; i++)
            free(meta->fullnames[i]);
        free(meta->fullnames);
    }
}

[astiob]

Adding free_font_info(&implicit_meta); and free(implicit_meta.postscript_name); at the end seems sensible to me. This looks like a regression in 887e6cc; @astiob any idea what happened there, or if there's any reason not to make the proposed change?

Yeah, no idea. Upon rereading the code, adding the proposed frees before the return true; seems right to me, too. @clsid2 Would you like to be credited as the author of the commit?

More optimally, we could selectively skip the copies later within the function if we’re copying from implicit_meta.

Perhaps a cleaner approach would be to initialize meta (get_font_info_IDWriteFontFace3(face3, &meta) for example), or (if that fails), call a separate function to initialize meta (using the if-block code from add_font_face). Once meta is successfully initialized, add_font_face can be called with a reliable meta.

This sounds like the opposite of 887e6cc and the way things were before that commit (save for any potential deduplication of the frees). I don’t fully remember why I chose to move things this way, but it probably had to do with simplifying code within the callers/font providers (ass_coretext and ass_directwrite)—e. g. see the ass_coretext diff in that same commit, and this commit was done in preparation for introducing the newer ass_directwrite code, which I must’ve expected to be even uglier.

[clsid2]

No need for credit.