Skip to content

Fix potential memory leaks in webpsave #4107

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

Merged
merged 11 commits into from
Aug 24, 2024

Conversation

dloebl
Copy link
Contributor

@dloebl dloebl commented Aug 23, 2024

Found while fuzzing locally with #4103:

==14==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1296 byte(s) in 1 object(s) allocated from:
    #0 0x56296b629ad8 in calloc /src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:77:3
    #1 0x56296c1fa633 in WebPAnimEncoderNewInternal /src/libwebp/src/mux/anim_encode.c:250:27
    #2 0x56296b79958a in WebPAnimEncoderNew /work/include/webp/mux.h:481:10
    #3 0x56296b79958a in vips_foreign_save_webp_init_anim_enc /src/libvips/build/../libvips/foreign/webpsave.c:658:14
    #4 0x56296b798c63 in vips_foreign_save_webp_build /src/libvips/build/../libvips/foreign/webpsave.c:780:7
    #5 0x56296b79c5c1 in vips_foreign_save_webp_target_build /src/libvips/build/../libvips/foreign/webpsave.c:979:6

To Reproduce

vips copy test/test-suite/images/invalid_multiframe.gif[n=-1] out.webp

ToDo:

  • Add a test

@dloebl dloebl marked this pull request as draft August 23, 2024 13:39
@dloebl dloebl marked this pull request as ready for review August 23, 2024 17:12
@dloebl dloebl changed the title Fix potential memory leaks in webpsave Fix possible memory leaks in webpsave Aug 24, 2024
@dloebl dloebl changed the title Fix possible memory leaks in webpsave Fix potential memory leaks in webpsave Aug 24, 2024
Copy link
Member

@jcupitt jcupitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about adding a call to vips_foreign_save_webp_unset() to vips_foreign_save_webp_dispose()? Then we could get rid of all these other calls on the error paths, since the operation will always be disposed pretty rapidly during error handling.

We could leave a call at the end of _build(), since it can potentially free a useful amount of memory that won't otherwise get disposed for a long time.

@dloebl
Copy link
Contributor Author

dloebl commented Aug 24, 2024

Sounds good! I've simplified the cleanup handling with 1af0ed7

@dloebl dloebl requested a review from jcupitt August 24, 2024 12:06
Copy link
Member

@jcupitt jcupitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh much safer and prettier!

@kleisauke
Copy link
Member

Perhaps this should target the 8.15 branch instead?

@dloebl dloebl changed the base branch from master to 8.15 August 24, 2024 12:39
@dloebl dloebl changed the base branch from 8.15 to master August 24, 2024 12:43
@dloebl dloebl force-pushed the fix-potential-leaks-webpsave branch from a36c89a to 1af0ed7 Compare August 24, 2024 12:46
@dloebl dloebl changed the base branch from master to 8.15 August 24, 2024 12:46
@dloebl
Copy link
Contributor Author

dloebl commented Aug 24, 2024

Hmm strange, I wonder why the macOS test-suite step is failing now.. it passed in my workflow: https://github.com/dloebl/libvips/actions/runs/10540080027/job/29204347884

pyvips.error.Error: unable to call webpsave_buffer

Maybe a retry helps

@kleisauke
Copy link
Member

I could reproduce that failure with libwebp 1.3.2 using this test script:

check-regression.sh:

#!/usr/bin/env bash
set -ex

n=$1
exit_code=0

for (( i = 0; i < n; i++ )); do
  echo "run $i"
  vips webpsave_buffer test/test-suite/images/invalid_multiframe.gif[n=-1] || exit_code=$?
  if [ $exit_code -ne 0 ]; then
    echo "vips exited with non-zero exit code ($exit_code) after $i runs" >&2
    exit $exit_code
  fi
done
$ ./check-regression.sh 1024
[...]
vips exited with non-zero exit code (1) after 63 runs

Curious.

@dloebl
Copy link
Contributor Author

dloebl commented Aug 24, 2024

Yeah, it's strange.. the test is definitely flaky.
It fails with gifload: Invalid frame data from time to time.

@dloebl
Copy link
Contributor Author

dloebl commented Aug 24, 2024

Maybe some sort of timing issue with threads?
It always fails on the first run with --vips-concurrency=1:

vips --vips-concurrency=1 webpsave_buffer 'test/test-suite/images/invalid_multiframe.gif[n=-1]'
[..]
echo 'vips exited with non-zero exit code (1) after 0 runs'

@dloebl
Copy link
Contributor Author

dloebl commented Aug 24, 2024

I'll remove the test. There is a test case in #4103 that also provides coverage: 1228b78

@dloebl dloebl force-pushed the fix-potential-leaks-webpsave branch from 7b5413e to 2089e9e Compare August 24, 2024 19:12
Copy link
Member

@kleisauke kleisauke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Flaky test will be fixed via PR #4116.

@kleisauke kleisauke merged commit d6186cf into libvips:8.15 Aug 24, 2024
6 checks passed
@dloebl dloebl deleted the fix-potential-leaks-webpsave branch August 25, 2024 15:54
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

Successfully merging this pull request may close these issues.

3 participants