Bounds-check AbstractMemory#__copy_from__ - #1193

Summary

AbstractMemory#__copy_from__ performed a memcpy with no bounds check on either operand, so any length larger than the destination was an out-of-bounds write and any length larger than the source was an out-of-bounds read.

static VALUE
memory_copy_from(VALUE self, VALUE rbsrc, VALUE rblen)
{
    AbstractMemory* dst;
    TypedData_Get_Struct(self, AbstractMemory, &rbffi_abstract_memory_data_type, dst);
    memcpy(dst->address, rbffi_AbstractMemory_Cast(rbsrc, &rbffi_abstract_memory_data_type)->address, NUM2INT(rblen));
    return self;
}

The length was only narrowed to int; nothing validated it against dst->size or src->size. A plain positive length was enough — no raw pointer, no manual memory management and no attach_function were involved.

This also reached user code that never calls __copy_from__ directly. FFI::StructLayout::InnerStruct#put (lib/ffi/struct_layout.rb:77) copies the declared field size out of value.pointer without checking that the source struct is actually backed by that many bytes, so a nested struct assignment outer[:inner] = inner read past the end of the source allocation.

Reproduction

require 'ffi'

src = FFI::MemoryPointer.new(:char, 65536)
dst = FFI::MemoryPointer.new(:char, 8)
dst.__copy_from__(src, 4096)          # 4096 byte write into an 8 byte allocation

Before this change, on Linux x86-64 with Ruby 4.0.6 this segfaulted. Under AddressSanitizer:

ERROR: AddressSanitizer: heap-buffer-overflow
    #0 memcpy
    #1 memory_copy_from ext/ffi_c/AbstractMemory.c:688

The nested struct path, which needs no direct __copy_from__ call:

class Inner < FFI::Struct
  layout :a, [:char, 4096]
end
class Outer < FFI::Struct
  layout :inner, Inner
end

inner = Inner.new(FFI::MemoryPointer.new(:char, 8))   # 4 KiB struct backed by 8 bytes
Outer.new[:inner] = inner

ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 4096
    #0 memcpy
    #1 memory_copy_from ext/ffi_c/AbstractMemory.c:688
    #10 struct_aset ext/ffi_c/Struct.c:402

Note that this second case did not crash in an uninstrumented build — it silently read adjacent heap memory.

Change

memory_copy_from now validates both operands with the same helpers the other accessors use:

    checkWrite(dst);
    checkBounds(dst, 0, len);
    checkRead(src);
    checkBounds(src, 0, len);

checkBounds tests (off | len | (off + len) | (size - (off + len))) < 0, so it rejects negative lengths as well as over-long ones. Pointers created from a raw address keep size == LONG_MAX, so they still pass and the deliberately unchecked raw-pointer primitives are unaffected.

The length conversion changed from NUM2INT to NUM2LONG so that range validation is handled uniformly by checkBounds rather than split between the conversion and the check, matching every other offset and length in AbstractMemory.c.

lib/ffi/struct_layout.rb is deliberately left alone. The legitimate nested-struct path slices the destination to exactly the field size and passes a source that is at least that large, so it satisfies both checks unchanged; guarding the sink rather than that one caller also covers anything else that reaches __copy_from__.

Both cases above now raise IndexError: Memory access offset=0 size=... is out of bounds.

Behaviour changes

• A length larger than either operand now raises IndexError instead of corrupting memory.
• A length above INT_MAX now raises IndexError rather than RangeError, because the conversion widened to NUM2LONG.
• __copy_from__ on a frozen (freezed, i.e. MEM_WR cleared) memory object now raises, consistent with put_bytes and the other writers.

Tests

__copy_from__ had no spec coverage at all before this. Added to spec/ffi/pointer_spec.rb:

• copies the requested number of bytes
• raises IndexError when the length exceeds the destination
• raises IndexError when the length exceeds the source
• raises IndexError for a negative length

Added to spec/ffi/struct_spec.rb:

• assigning a whole struct to a nested field copies its value
• raises IndexError when a struct assigned to a nested field is backed by too little memory

Cross-engine results

Each example was run on all three engines before deciding whether to guard it, rather than assuming C-extension-only. The two ends came out differently:

• The positive invariant is genuinely cross-engine. Assigning a struct to a nested field works identically on CRuby, JRuby and TruffleRuby, so that example is left unguarded and acts as a regression test on all three.
• The raise-based examples are not, and the reasons differ per engine:
  • JRuby does not implement __copy_from__ at all (NoMethodError), so the whole #__copy_from__ block is skipped there. Separately, JRuby already rejects the undersized nested struct — but at construction time, with ArgumentError: memory object has insufficient space, not at assignment — so the nested-struct example does not fit it either.
  • TruffleRuby implements __copy_from__ and the plain copy works, so that example runs there. But it performs no bounds check: an 8-byte destination accepts a 4096-byte copy silently, the undersized nested struct assignment silently reads out of bounds, and a negative length reaches Unsafe and escapes as a Java IllegalArgumentException that terminates the interpreter. Those three are skipped there.

Suite results on this branch:

The single JRuby failure is library_spec.rb:105 ("interprets INPUT() in linker scripts"), which fails identically on a tree without this change — a pre-existing environment-dependent failure in my local setup, not a regression.

Verification

Verified on Linux x86-64, Ruby 4.0.6, system libffi 3.7.1, with an AddressSanitizer/UndefinedBehaviorSanitizer build of the extension. Both reproductions above report heap-buffer-overflow before the change and raise IndexError with no sanitizer output after it. The only remaining sanitizer output is the pre-existing misaligned-load report at MethodHandle.c:281, which fires on any attach_function call and is unrelated.

The trigger is data-model independent — a small concrete length is enough — so LLP64 needs no separate derivation.

Notes for review

1. Exception class for huge lengths. Widening NUM2INT to NUM2LONG means a length above INT_MAX now raises IndexError instead of RangeError. __copy_from__ is effectively internal — struct_layout.rb is its only in-tree caller — but it is a public method, so this is worth a conscious nod.
2. checkWrite on frozen memory. __copy_from__ on a frozen memory object now raises, matching the other writers. Called out in case the omission was deliberate.
3. Guarding the sink rather than the caller. InnerStruct#put could have been fixed in Ruby instead. Guarding __copy_from__ was chosen so the check applies to every caller; happy to add a Ruby-side check as well if you would rather have a clearer error message at that layer.
4. TruffleRuby has the same defect, unfixed. As noted above, its __copy_from__ performs no bounds check and silently writes out of bounds. Not this PR's to fix, but it seems worth reporting upstream — I have not filed anything.

🤖 Generated with Claude Code