Fix HEIF image handling with uninitialized library #895
Conversation
da98241 to
d3527a0
Compare
There was a problem hiding this comment.
we can't expose format-specific init functions like this. no one will call them, and we don't want multiple inits. I'm not sure there's no point in calling deinit at all? in which case, can't we have the heif backend call the init function itself on demand and that's it?
src/gd_heif.c
Outdated
| @@ -23,13 +23,52 @@ | |||
| #define GD_HEIF_ALLOC_STEP (4*1024) | |||
| #define GD_HEIF_HEADER 12 | |||
|
|
|||
| static int heifInit = 0; | |||
There was a problem hiding this comment.
Maybe more importantly, that won't work for multi-threading, I think.
There was a problem hiding this comment.
neither type works for multithreading. they'd need to be atomic and/or grab pthread locks.
that said, I don't think we've ever thought about MT, so I wouldn't be surprised if there were already issues in the codebase.
There was a problem hiding this comment.
I'm not aware 8000 of any multi-threading issues in libgd when used with PHP. :)
There was a problem hiding this comment.
gdkanji.c is not MT safe. none of the error handler APIs are MT safe (which is to say, the handlers are per-process, not per-thread). I guess most MT issues after that would be library-specific and not really on gd's plate.
maybe along those lines, should we punt these problems completely onto the user ? if the only thing gdHeifInit does is call heif_init, and the only reason people would call gdHeifInit is because they want to explicitly use the HEIF format, then why not force them to call heif_init themselves ? that would also let them call heif_deinit if they really wanted to.
if we can't handle it automatically in a format/library agnostic way, i don't think we're bringing any value-add here.
|
First, thank you for the PR @YakoYakoYokuYoku!
Well, they may release resources there, so it would be nice to call the function. |
src/gd_heif.c
Outdated
| @@ -23,13 +23,52 @@ | |||
| #define GD_HEIF_ALLOC_STEP (4*1024) | |||
| #define GD_HEIF_HEADER 12 | |||
|
|
|||
| static int heifInit = 0; | |||
There was a problem hiding this comment.
Maybe more importantly, that won't work for multi-threading, I think.
src/gd_heif.c
Outdated
| if (gdHeifInit()) { | ||
| gd_error("gd-heif failure to initialize library"); | ||
| return GD_FALSE; | ||
| } |
There was a problem hiding this comment.
Indentation seems to be off here (and below).
d3527a0 to
8ab7191
Compare
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. |
I haven't checked the amount of resources which would not be freed, but given that 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 |
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.
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
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. |
From version
1.13.0of libheif the library needs to be initialized to load its plugins and it has to be done by any consumer of the library, including libgd, I've forgot to add such thing. This PR adds two function for it,gdHeifInitandgdHeifDeinit, the former gets called by any method that handles HEIF images and the latter is used for cleanup. Fixes #889.