Fix HEIF image handling with uninitialized library - #895

First, thank you for the PR @YakoYakoYokuYoku!

I'm not sure there's no point in calling deinit at all?

Well, they may release resources there, so it would be nice to call the function.

Well, they may release resources there, so it would be nice to call the function.

i grok the hypothetical, but i question the realistic/practical aspects. i still don't think people will (or reasonably can) call format-specific init/deinit functions. the user of gd (e.g. php code) can be disconnected from the builder of gd (e.g. the server/distro), and the code can be written in a way that is format agnostic so users could upload arbitrary content.

calling format-specific init automatically is fairly easy for us in gd since it'd be a one-time event, but calling deinit & re-init could easily be difficult to get right. if it's a loop processing multiple images, do we deinit when the image is written out or destroyed ? do we do some refcounting in case multiple images are loaded at once ? do we do some basic GC to delay ?

unless the deinit releases a significant amount of resources, I argue that we should never do it, nor expose an API for it.

unless the deinit releases a significant amount of resources, I argue that we should never do it, nor expose an API for it.

From https://github.com/strukturag/libheif/blob/33ec3d63470944906dbccffa085970083969de3a/libheif/api/libheif/heif.h#L544-L545:

For backwards compatibility, it is not really necessary to call heif_init(), but some library memory objects will never be freed if you do not call heif_init()/heif_deinit().

I haven't checked the amount of resources which would not be freed, but given that heif_init() and heif_deinit() are only available as of libheif 1.13.0, we cannot call the functions unconditionally anyway, unless we raise the libheif requirements (so far we require libheif >= 1.7.0).

Given that there are more urgent issues regarding libgd than some memory leak at the end of the process (well, usually only there), I think we should postpone calling these functions. And the cleaner solution would likely be to introduce some gdInit()/gdFinalize() first, but these may have their own issues (and wouldn't be available with older libgd).

I haven't checked the amount of resources which would not be freed

Looking at the implementation, it unloads any loaded external plugins, and clears a colorspace LUT. It doesn't seem like those alone are a big enough deal for us to care about.

Really the only concerning thing is what @YakoYakoYokuYoku noted at the top: if you don't call init, then no external plugins will be loaded.

but given that heif_init() and heif_deinit() are only available as of libheif 1.13.0, we cannot call the functions unconditionally anyway, unless we raise the libheif requirements (so far we require libheif >= 1.7.0).

If the libheif version is available at compile time, we can check that. Or we can add configure-time checks for the functions and use them if available. I won't seriously suggest probing via dlopen and related APIs.

And the cleaner solution would likely be to introduce some gdInit()/gdFinalize() first.

I'm still on the side of doing nothing here. This only affects people using libheif with external plugins, and those people are free to call init themselves. Otherwise it's extra boilerplate that is a nop, and decades of existing GD code prob won't update to use it.

The libheif API says its init/deinit calls are refcounted, so we could call init safely everytime HEIF APIs are used, but we wouldn't call deinit, so we'd be leaking that logic. Which, practically speaking, is what we already do with older versions, so arguably that's maintaining status quo.

Should we not rather add a gd main init, and add whatever is needed in there?

InitOnceExecuteOnce on windows
pthread_once on posix
not sure about other.

For shared uses, document it, explain if one uses dl/LoadLibrary, unload it and load it again, it is required to call that function again "manually" as the underlying kernel won't do it.

for static link usage, caller must call that function. Nothing we can do there (like in php f.e.) but that's easy as php has a per process/thread init where needed, easy to add in the ext/gd

Alternatively, it could be done on demand, add a check if it has been initialized in _gdImageHeifCtx, if not init, return on error, continue if it was successful? same code as now but no public function exposed yet, as it is, if I remember correctly, the only requiring an init, maybe Freetype too, I have to check.