-
Notifications
You must be signed in to change notification settings - Fork 62
Vips.block seems to not work as expected
#410
-
Hi!
I'm not sure whether this is a bug, but Vips.block does not seem to work as intended.
Indeed, our codebase uses Vips.block('VipsForeign', true) then Vips.block('VipsForeignLoadJpeg', false) and others to re-enable a specific list of loaders, but a user reports this is broken on their deployment: mastodon/mastodon#33809 (comment)
According to them, they are unable to process JPEG files when Vips.block('VipsForeignLoadJpeg', false) is called, even when not calling Vips.block('VipsForeign', true).
I am not sure what is going there here.
Beta Was this translation helpful? Give feedback.
All reactions
Replies: 5 comments 10 replies
-
Hi @ClearlyClaire, that's odd, I'll have a look.
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1
-
I tried a test program:
#!/usr/bin/ruby require "vips" # block all loaders and savers Vips.block("VipsForeign", true) # reenable jpeg load Vips.block("VipsForeignLoadJpeg", false) image = Vips::Image.new_from_file(ARGV[0]) puts "image is #{image.width} x #{image.height} pixels"
I see:
john@orange ~/try $ ./block.rb ~/pics/k2.jpg
image is 1450 x 2048 pixels
john@orange ~/try $ ./block.rb ~/pics/k2.png
/home/john/packages/gems/gems/ruby-vips-2.2.2/lib/vips/image.rb:281:in `new_from_file': VipsForeignLoad: "/home/john/pics/k2.png" is not a known file format (Vips::Error)
from ./block.rb:11:in `<main>'
So it seems to work for me (ruby 3.2, ruby-vips 2.2, libvips 8.16).
You'll need to find a way to reproduce this error. Is it possible to get more information from the user?
Beta Was this translation helpful? Give feedback.
All reactions
-
I'll tag them here!
@17dec can you try running the above script, see how it behaves, and give us any relevant information?
Beta Was this translation helpful? Give feedback.
All reactions
-
Interesting, I'm getting unreliable results when running that script:
$ bundle exec ./test.ruby ./spec/fixtures/files/attachment.jpg
image is 600 x 400 pixels
$ bundle exec ./test.ruby ./spec/fixtures/files/attachment.jpg
$REDACTED/bundle/ruby/3.2.0/gems/ruby-vips-2.2.2/lib/vips/image.rb:281:in `new_from_file': VipsForeignLoad: "./spec/fixtures/files/attachment.jpg" is not a known file format (Vips::Error)
from ./test.ruby:11:in `<main>'
$ bundle exec ./test.ruby ./spec/fixtures/files/attachment.jpg
$REDACTED/bundle/ruby/3.2.0/gems/ruby-vips-2.2.2/lib/vips/image.rb:281:in `new_from_file': VipsForeignLoad: "./spec/fixtures/files/attachment.jpg" is not a known file format (Vips::Error)
from ./test.ruby:11:in `<main>'
$ bundle exec ./test.ruby ./spec/fixtures/files/attachment.jpg
image is 600 x 400 pixels
Valgrind confirms there's an uninitialized read:
==14191== Conditional jump or move depends on uninitialised value(s)
==14191== at 0xCBA9683: vips_operation_block_set_operation (in /usr/lib64/libvips.so.42.18.0)
==14191== by 0xCB9AB4E: vips_class_map_all (in /usr/lib64/libvips.so.42.18.0)
==14191== by 0xCB9AA8F: vips_type_map (in /usr/lib64/libvips.so.42.18.0)
==14191== by 0xCBABCA4: vips_operation_block_set (in /usr/lib64/libvips.so.42.18.0)
==14191== by 0x56B7ABD: ffi_call_unix64 (in /usr/lib64/libffi.so.8.1.4)
==14191== by 0x56B6DBD: ffi_call_int (in /usr/lib64/libffi.so.8.1.4)
==14191== by 0x56B75AA: ffi_call (in /usr/lib64/libffi.so.8.1.4)
==14191== by 0xCA21028: rbffi_CallFunction (in $REDACTED/bundle/ruby/3.2.0/gems/ffi-1.16.3/lib/ffi_c.so)
==14191== by 0xCA252F5: custom_trampoline (in $REDACTED/bundle/ruby/3.2.0/gems/ffi-1.16.3/lib/ffi_c.so)
==14191== by 0x4AB655C: vm_call_cfunc_with_frame (in /usr/lib64/libruby32.so.3.2.5)
==14191== by 0x4AC456F: vm_exec_core (in /usr/lib64/libruby32.so.3.2.5)
==14191== by 0x4AC98CD: rb_vm_exec (in /usr/lib64/libruby32.so.3.2.5)
(Although I'm not sure how reliable that message is, it's also showing a lot of uninitialized reads within ruby itself. Those are all in the garbage collector, it looks like, so maybe that's to be expected)
Not really sure how that happens. FFI confusion over C 'bool' vs. 'gboolean'? Then again, I kind of expect both to expand to an 'int' in terms of ABI.
Beta Was this translation helpful? Give feedback.
All reactions
-
Your c version works reliably and valgrind output is nice and clean on my system as well.
$ ./block test.jpg
width = 512
$ ./block test.png
VipsForeignLoad: "test.png" is not a known file format
I only have a single libvips binary:
$ find / -xdev -name 'libvips*.so'
/usr/lib64/libvips-cpp.so
/usr/lib64/libvips.so
I rebuilt ruby with valgrind instrumentation (just found out Gentoo offers that option), and with that valgrind is almost completely silent on the ruby test script. The unconditional read in vips_operation_block_set_operation is the one thing that remains.
Some gentoo package versions & configuration:
media-libs/vips-8.16.0::gentoo was built with the following:
USE="heif highway jpeg jpeg2k jpegxl lcms png webp -archive -deprecated -doc -exif -fftw -fits -fontconfig -graphicsmagick -gtk-doc -imagemagick -imagequant -introspection -matio -openexr -orc -pango -pdf -python -svg -test -tiff -vala" ABI_X86="(64)" PYTHON_SINGLE_TARGET="python3_11 -python3_10 -python3_12 -python3_13"
dev-libs/libffi-3.4.6-r2::gentoo was built with the following:
USE="-debug -exec-static-trampoline -pax-kernel -static-libs -test" ABI_X86="32 (64) (-x32)"
dev-lang/ruby-3.2.5-r2::gentoo was built with the following:
USE="ssl valgrind -berkdb -debug -doc -examples -gdbm (-jemalloc) -jit -socks5 (-static-libs) -systemtap -tk -xemacs" ABI_X86="(64)"
CFLAGS="-O2 -pipe -march=native -mtune=native -fno-strict-aliasing"
CXXFLAGS="-O2 -pipe -march=native -mtune=native -fno-strict-aliasing"
sys-devel/gcc-14.2.1_p20241221::gentoo was built with the following:
USE="cet (cxx) (default-stack-clash-protection) (default-znow) fortran (multilib) openmp (pie) sanitize ssp zstd -ada (-custom-cflags) -d -debug -doc (-fixed-point) -go -graphite -hardened (-ieee-long-double) -jit (-libssp) -lto -modula2 -nls -objc -objc++ -objc-gc (-pch) -pgo -rust -systemtap -test (-time64) -valgrind -vanilla -vtv" ABI_X86="(64)"
CFLAGS="-pipe -march=native -mtune=native -O2"
CXXFLAGS="-pipe -march=native -mtune=native -O2"
sys-libs/glibc-2.40-r8::gentoo was built with the following:
USE="cet multiarch (multilib) ssp stack-realign (static-libs) -audit -caps -compile-locales (-custom-cflags) -doc -gd -hash-sysv-compat -headers-only -multilib-bootstrap -nscd -perl -profile (-selinux) -suid -systemd -systemtap -test (-vanilla)" ABI_X86="(64)"
I rebuilt ruby, vips and libffi just in case gcc/glibc/anything-else-important has changed since they were last built, but that didn't change anything.
Another observation: it seems the uninitialized read is only triggered when 'false' is passed as second argument. No problems are reported when the second argument is 'true'.
Beta Was this translation helpful? Give feedback.
All reactions
-
FFI confusion over C 'bool' vs. 'gboolean'? Then again, I kind of expect both to expand to an 'int' in terms of ABI.
ruby-vips uses int for other gboolean function definitions, since that is a typedef for int in GLib.
Line 97 in 7a2b9ba
Line 264 in 7a2b9ba
https://github.com/GNOME/glib/blob/2.83.3/glib/gtypes.h#L56
To rule things out, are you able to test commit kleisauke@ec69676?
Beta Was this translation helpful? Give feedback.
All reactions
-
To rule things out, are you able to test commit kleisauke@ec69676?
Can confirm that that commit fixes the problem. With that, the script works reliably and no uninitialized reads are reported.
Beta Was this translation helpful? Give feedback.
All reactions
-
To rule things out, are you able to test commit kleisauke@ec69676?
Can confirm that that commit fixes the problem. With that, the script works reliably and no uninitialized reads are reported.
Great! I just opened PR #411 for this with a polished commit message and added changelog note.
I'm not sure why I can't reproduce this locally, you would expect this to cause issues on other platforms too.
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1
-
Well done Kleis! You'd think ruby-ffi would pass bools as ints, that's very unexpected.
Beta Was this translation helpful? Give feedback.
All reactions
-
So that's just bizarre. Let's do ruby-vips 2.2.3 with this fix ASAP.
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1
-
OK, 2.2.3 with this fix is up. Thank you Kleis! And thanks for the report and feedback @ClearlyClaire and @17dec
Beta Was this translation helpful? Give feedback.
All reactions
-
👍 1
-
No problem! Could you also push commit 9fb56ce to the master branch? I noticed v2.2.3 was based on that, but the commit itself is not (yet) present on the main branch.
Beta Was this translation helpful? Give feedback.
All reactions
-
Oops! Sorry. I always expect git push --tags to push code as well.
Beta Was this translation helpful? Give feedback.