Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle out-of-memory errors, such as avifAlloc() failures #820

Closed
wantehchang opened this issue Jan 21, 2022 · 11 comments
Closed

Handle out-of-memory errors, such as avifAlloc() failures #820

wantehchang opened this issue Jan 21, 2022 · 11 comments
Assignees

Comments

@wantehchang
Copy link
Collaborator

This issue tracks the work on handling out-of-memory errors, starting with avifAlloc() failures.

I will do this work in installments because this requires a lot of changes.

@wantehchang wantehchang self-assigned this Jan 21, 2022
wantehchang added a commit to wantehchang/libavif that referenced this issue Jan 21, 2022
Add the AVIF_RESULT_OUT_OF_MEMORY error code.

Add the avifArrayPop() function to remove the last element in the array.

Bug: AOMediaCodec#820.
wantehchang added a commit to wantehchang/libavif that referenced this issue Jan 26, 2022
Add the AVIF_RESULT_OUT_OF_MEMORY error code.

Add the avifArrayPop() function to remove the last element in the array.

Bug: AOMediaCodec#820.
wantehchang added a commit to wantehchang/libavif that referenced this issue Jan 26, 2022
Add the AVIF_RESULT_OUT_OF_MEMORY error code.

Add the avifArrayPop() function to remove the last element in the array.

Bug: AOMediaCodec#820.
wantehchang added a commit to wantehchang/libavif that referenced this issue Jan 26, 2022
Add the AVIF_RESULT_OUT_OF_MEMORY error code.

Add the avifArrayPop() function to remove the last element in the array.

Bug: AOMediaCodec#820.
wantehchang added a commit that referenced this issue Jan 27, 2022
Add the AVIF_RESULT_OUT_OF_MEMORY error code.

Add the avifArrayPop() function to remove the last element in the array.

Bug: #820.
wantehchang added a commit to wantehchang/libavif that referenced this issue Jan 29, 2022
Fix a bug introduced in commit f732a4d.

In avifEncoderDataCreateItem(), increment data->lastItemID before
setting item->id equal to data->lastItemID. This is what the original
code did.

Bug: AOMediaCodec#820
wantehchang added a commit that referenced this issue Jan 30, 2022
Fix a bug introduced in commit f732a4d.

In avifEncoderDataCreateItem(), increment data->lastItemID before
setting item->id equal to data->lastItemID. This is what the original
code did.

Fix #836.

Bug: #820
wantehchang added a commit to wantehchang/libavif that referenced this issue Feb 14, 2022
Allow avifDecoderDataCreateTile() to fail on avifImageCreateEmpty() or
avifCodecDecodeInputCreate() failures. Note that we don't check for
avifArrayPushPtr() failure yet. That will be done later.

Change all avifDecoderDataCreateTile() callers to handle its failures.

Bug: AOMediaCodec#820
wantehchang added a commit that referenced this issue Feb 14, 2022
Allow avifDecoderDataCreateTile() to fail on avifImageCreateEmpty() or
avifCodecDecodeInputCreate() failures. Note that we don't check for
avifArrayPushPtr() failure yet. That will be done later.

Change all avifDecoderDataCreateTile() callers to handle its failures.

Bug: #820
y-guyon added a commit to y-guyon/libavif that referenced this issue Apr 5, 2022
This is part of a series of changes to replace abort() by return null
upon memory allocation failure in avifAlloc().

Bug: AOMediaCodec#820
wantehchang pushed a commit that referenced this issue Apr 5, 2022
This is part of a series of changes to replace abort() by return null
upon memory allocation failure in avifAlloc().

Bug: #820
y-guyon added a commit to y-guyon/libavif that referenced this issue Jul 26, 2023
Also avifImageSetProfileICC(), avifImageSetMetadataExif(),
avifImageSetMetadataXMP() and avifCodecEncodeOutputAddSample().
To catch out-of-memory issues.
Bug: AOMediaCodec#820
y-guyon added a commit that referenced this issue Jul 31, 2023
Also avifImageSetProfileICC(), avifImageSetMetadataExif(),
avifImageSetMetadataXMP() and avifCodecEncodeOutputAddSample().
To catch out-of-memory issues.
Bug: #820
wantehchang added a commit to wantehchang/libavif that referenced this issue Aug 23, 2023
On memory allocation failure, the current behavior is to abort the
process. We are working to change avifAlloc() to return NULL. See
AOMediaCodec#820.
wantehchang added a commit that referenced this issue Aug 23, 2023
On memory allocation failure, the current behavior is to abort the
process. We are working to change avifAlloc() to return NULL. See
#820.
@y-guyon
Copy link
Collaborator

y-guyon commented Oct 19, 2023

All avifAlloc() calls should be checked now.

@vrabaud
Copy link
Collaborator

vrabaud commented Oct 20, 2023

I confirm all avifAlloc are checked.

@wantehchang wantehchang assigned y-guyon and unassigned wantehchang and y-guyon Oct 20, 2023
@wantehchang
Copy link
Collaborator Author

Yannis: Thank you very much for all your work on this issue! This is an enormous amount of work and requires patience and perseverance.

I changed the assignee to you and removed myself. (The initial work I did is insignificant compared with what you have done.)

Does this mean we can remove the abort() call in avifAlloc() now?

@y-guyon
Copy link
Collaborator

y-guyon commented Oct 20, 2023

Thanks.

Does this mean we can remove the abort() call in avifAlloc() now?

Yes. I was wondering if adding a test or fuzz target to exercise malloc() failures would be worth it. Maybe it can be done after the change.

@y-guyon
Copy link
Collaborator

y-guyon commented Oct 20, 2023

Once the changes above are merged, the apps and src folders are handled.

There are a few allocations in third_party/libyuv (a small portion of code copied from upstream libyuv). These are not checked.
Unfortunately it also means using the whole libyuv library as a dependency is a liability. I filed https://bugs.chromium.org/p/libyuv/issues/detail?id=968.

I can imagine other dependencies such as libaom bringing the same risk. Should this bug be closed before all dependencies are safe?

@wantehchang
Copy link
Collaborator Author

I suggest closing this issue as completed and open a new issue about libyuv.

Unlike libyuv, libaom aims to handle memory allocation failures (see https://crbug.com/aomedia/3276), so it doesn't seem necessary to file a new issue about libaom.

@wantehchang
Copy link
Collaborator Author

wantehchang commented Oct 20, 2023

Sorry, I forgot the code in third_party/libyuv. In my previous comment, I was referring to the upstream libyuv.

We can probably also open a new issue about the code in third_party/libyuv. Or we can leave this issue open until that code is fixed.

@y-guyon
Copy link
Collaborator

y-guyon commented Oct 21, 2023

we can leave this issue open until that code is fixed.

I agree.

Unlike libyuv, libaom aims to handle memory allocation failures (see https://crbug.com/aomedia/3276), so it doesn't seem necessary to file a new issue about libaom.

Great. What about other dependencies, especially dav1d? I think libsharpyuv is fine too, right @vrabaud?

@vrabaud
Copy link
Collaborator

vrabaud commented Oct 23, 2023

Indeed, fr libsharpyuv, allocs are only done in one spot and they are checked: https://chromium.googlesource.com/webm/libwebp/+/refs/heads/main/sharpyuv/sharpyuv.c#331

@wantehchang
Copy link
Collaborator Author

Yannis: I randomly inspected a few calls to dav1d_malloc() and dav1d_alloc_aligned() in dav1d. It's clear that dav1d handles memory allocation failures.

@y-guyon
Copy link
Collaborator

y-guyon commented Nov 9, 2023

There are a few allocations in third_party/libyuv (a small portion of code copied from upstream libyuv). These are not checked.

Now that it is done, all core library places should be checked now.

@y-guyon y-guyon closed this as completed Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants