Memory allocation failures are not checked and handled. about libavif

[joedrago]

I can add AVIF_RESULT_OUT_OF_MEMORY and do an audit pass, trying to catch all calls to avifAlloc() and avifArrayPushPtr(). If I manage to catch them all, perhaps I'll change the policy to enforce this.

[joedrago]

I hadn't planned to catch failures to allocate memory, and while I'm sure I could plumb a lot of that through, I'd prefer not to (I could be convinced otherwise).

If I chose not to, how/where would you expect such documention to be?

[wantehchang]

README.md seems like a good place to document this policy. (It is the only .md file in the source tree.) Another good place would be the public header include/avif/avif.h.

[AurelC2G]

Any chance we can revisit this decision? On mobile especially we can run into situations when the available memory is very limited, and right now the current behavior of libavif would lead to hard-to-debug crashes due to memory corruption. Probably better to explicitly error out if an allocation failed.

[AurelC2G]

Alternatively, we could at least abort() within avifAlloc if malloc failed. This way we avoid undefined behavior of just dereferencing these null pointers.

[0xC0000054]

Alternatively, we could at least abort() within avifAlloc if malloc failed. This way we avoid undefined behavior of just dereferencing these null pointers.

I think that rav1e may already do this, based on the fact that Rust currently aborts the process when a memory allocation fails (see rust-lang/rust#43596) and the lack of an out-of-memory error code in the C API.

While calling abort() is preferable to derefrencing a null pointer, there are some use cases where it would be unacceptable to terminate the process.
For example, libavif could not be used to create an AVIF plugin for Adobe Photoshop because Photoshop plugins are loaded in-process, and terminating Photoshop on out-of-memory would cause the user to lose all of their images that have unsaved changes.

My Paint.NET AVIF plugin uses AVIF format read/write code written in C#, with AOM for encoding and decoding.
This allows me to report out out-of-memory conditions to the user instead of crashing, and it also uses less memory when loading and saving files.

I can add AVIF_RESULT_OUT_OF_MEMORY and do an audit pass, trying to catch all calls to avifAlloc() and avifArrayPushPtr(). If I manage to catch them all, perhaps I'll change the policy to enforce this.

As I mentioned above, this would depend on the behavior of the individual codecs.
It appears that AOM and dav1d return an error code for out-of-memory, so you may just have to verify and document which codecs support allowing the caller to handle out-of-memory conditions.

[AurelC2G]

The problem is not so much about the allocations performed by the codecs (out of scope for this repository), but the allocations performed by libavif itself.

While calling abort() is preferable to derefrencing a null pointer, there are some use cases where it would be unacceptable to terminate the process.
For example, libavif could not be used to create an AVIF plugin for Adobe Photoshop because Photoshop plugins are loaded in-process, and terminating Photoshop on out-of-memory would cause the user to lose all of their images that have unsaved changes.

I agree that abort is not ideal compared to reporting the errors and letting client code handle them. However, the current version is even worse, dereferencing a null pointer is undefined behavior so the outcome could be anything (including terminating the process).