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

avifdec writing PNG returns error #1086

Closed
FrankGalligan opened this issue Sep 4, 2022 · 5 comments
Closed

avifdec writing PNG returns error #1086

FrankGalligan opened this issue Sep 4, 2022 · 5 comments
Assignees

Comments

@FrankGalligan
Copy link
Contributor

Steps to reproduce:

  1. ./avifenc input.png output.avif
  2. ./avifdec output.avif new.png

I get error:
libpng error: profile 'libavif': 49002h: length does not match profile

If I comment out this line:
https://github.com/AOMediaCodec/libavif/blob/main/apps/shared/avifpng.c#L421

Then I do not see the error.

@wantehchang
Copy link
Collaborator

Frank: I can't reproduce this bug. Could you provide a sample PNG file? You can email it to me and Yannis. Thanks.

@y-guyon
Copy link
Collaborator

y-guyon commented Sep 12, 2022

I managed to reproduce it internally with avifenc -s 10 tests/data/paris_icc_exif_xmp.png + avifdec with no special flag. It seems to be a heap-use-after-free:

  1. avifDecoderParse() called by avifdec calls avifParse().
    1. avifParse() stores the pointer to the icc chunk within the output of decoder->io->read().
      decoder->io->persistent is set to AVIF_FALSE at that time.
  2. avifDecoderParse() calls avifDecoderReset().
    1. avifDecoderReset() calls avifDecoderFindMetadata() which calls avifDecoderItemRead(). It uses avifIOFileReaderRead() which extends the output of decoder->io->read() to fit more bytes.
    2. avifDecoderReset() copies the ICC bytes from prop->u.colr.icc (now an invalid pointer) to decoder->image->icc.

Step 1.i. should store the offset instead of the pointer, or copy the bytes immediately.

@y-guyon y-guyon self-assigned this Sep 12, 2022
wantehchang added a commit to wantehchang/libavif that referenced this issue Sep 15, 2022
Add a test for AOMediaCodec#1086 to
prevent regression.
wantehchang added a commit to wantehchang/libavif that referenced this issue Sep 15, 2022
Add a test for AOMediaCodec#1086 to
prevent regression.
@wantehchang
Copy link
Collaborator

Yannis: Good job tracking this bug down. This kind of memory error is a serious problem. We should take the opportunity to review the relevant code carefully. Do you know why the Exif and XMP metadata are not affected by this bug? It seems that they avoid this bug by either reading the data early or storing the offset (in the avifExtent struct), the same approach you used to fix the bug.

@y-guyon
Copy link
Collaborator

y-guyon commented Sep 16, 2022

I sent #1114 as an attempt to catch this kind of issue, when called by a fuzz target for example. A local run of an internal fuzz target did not report anything. Maybe the CI continuous fuzzing will.

wantehchang added a commit that referenced this issue Sep 17, 2022
Add a test for #1086 to
prevent regression.
@wantehchang
Copy link
Collaborator

Fixed by #1103.

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