Source.new_from_memory: keep a ref to the memory area - #438
Open
jeremy wants to merge 1 commit into
Open
Conversation
vips_source_new_from_memory() aliases the caller's buffer rather than copying it -- vips_blob_new(NULL, data, length), NULL free_fn -- so libvips reads it for as long as the source is alive. We kept no Ruby reference to it, so nothing stopped the GC from freeing it first. Mirrors Image.new_from_memory, which already does this. Also answers and removes the FIXME: Image.new_from_buffer does not need a ref. It goes through GValue#set for Vips::BLOB_TYPE, which g_mallocs its own block, memcpys into it and hands libvips ownership via vips_value_set_blob(..., GLib::G_FREE, ...).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Vips::Source.new_from_memoryhandsdatato libvips and keeps no Ruby reference to it:libvips aliases that buffer rather than copying it —
libvips/iofuncs/source.c:NULLfree_fn, so no ownership transfer: libvips reads the caller's bytes for as long as the source lives. Nothing on the Ruby side was keeping them alive, so the GC is free to collect the string first.Image.new_from_memorytwo files over already handles this —image.references << data. This does the same forSource.Answering the FIXME
The FIXME asks two questions. This PR answers both and removes it.
Does the source need a ref? Yes — demonstrated, not theoretical. New spec, which drops the caller's reference and calls
GC.startbefore loading (same shape as the existingImage.new_from_memoryspecs). On master, ruby 4.0.6 / arm64-darwin23 / libvips 8.18.4, it fails 14 times out of 20 runs, with the source's bytes visibly overwritten:With the ref, 0 failures out of 20.
Does
Image.new_from_bufferneed one too? No. It goes throughOperation.call, which sets the loader's buffer argument viaGValue#set, and forVips::BLOB_TYPEthat allocates a fresh block and copies into it (lib/vips/gvalue.rb):The
GLib::G_FREEfree_fn hands the copy to libvips, so libvips owns it and the caller's string is irrelevant afterwards. This is worth stating explicitly because it's easy to conclude the opposite:Operation.call'sdeduped_referencesmachinery only propagatesreferencesbetweenVips::Imageinputs and outputs and does nothing for a plainStringbuffer, so reading only that code makesnew_from_bufferlook unprotected. The protection is one level down, inGValue#set.Scope and severity
lib/vips/source.rbis byte-identical (md5eb69926777374771d845a579151c4cb0) in 2.2.2, 2.2.5, 2.3.0 and current master, so this has been present unchanged across all of them.There are no callers of
Source.new_from_memoryinside the gem itself, and none in our own applications — this is a latent defect being fixed preemptively, not a live incident.One limitation worth naming:
references <<fixes liveness only, not movement. A Ruby String at or below the embedded boundary keeps its bytes inside the object slot and relocates under GC compaction, so a live ref wouldn't stop the pointer going stale. I measured that boundary at 616 bytes on ruby 3.4.7, 3.4.8, 3.4.10 and 4.0.6 (ObjectSpace.dump(s)["embedded"]flips at"a" * 616). It doesn't matter for real image buffers, which are far above it — but it would if a small-buffer path were ever added.Also not addressed here, but adjacent:
Image.new_from_memoryhas a JRuby branch that copies the string into anFFI::MemoryPointerfirst, because JRuby's FFI passes a temporary native buffer for a Ruby String rather than the string's own storage.Source.new_from_memoryhas no equivalent, and a Ruby-side ref doesn't help there. I left it alone rather than add a branch I can't exercise locally — happy to follow up if you'd like it.Tests
Full suite on ruby 4.0.6 / arm64-darwin23 / libvips 8.18.4:
standardrbclean on both changed files.