-
-
Notifications
You must be signed in to change notification settings - Fork 699
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
Conversation
There was a problem hiding this 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.
Sounds good! I've simplified the cleanup handling with 1af0ed7 |
There was a problem hiding this 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!
Perhaps this should target the |
a36c89a
to
1af0ed7
Compare
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
Maybe a retry helps |
I could reproduce that failure with libwebp 1.3.2 using this test script:
#!/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. |
Yeah, it's strange.. the test is definitely flaky. |
Maybe some sort of timing issue with threads?
|
7b5413e
to
2089e9e
Compare
There was a problem hiding this 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.
Found while fuzzing locally with #4103:
To Reproduce
ToDo: