The callbacks trim, zero, extents and cache pass an int64 offset to the OCaml code. However we were using caml_copy_int32 to create this value which is obviously incorrect. (This would work on little endian machines for offsets < 4G, unfortunately, which is why we didn't spot this. Also it is not covered by any tests.) This mistake goes right back to the original commit which added the plugin, commit e3abac4e55 ("ocaml: New plugin lets you write plugins as natively compiled OCaml programs.") Fixes: commit e3abac4e5519b0d51c9a34a41af97f63920f3f32 --- plugins/ocaml/plugin.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/ocaml/plugin.c b/plugins/ocaml/plugin.c index 53b23eb7..b6964046 100644 --- a/plugins/ocaml/plugin.c +++ b/plugins/ocaml/plugin.c @@ -657,7 +657,7 @@ trim_wrapper (void *h, uint32_t count, uint64_t offset, uint32_t flags) LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE (); countv = caml_copy_int32 (count); - offsetv = caml_copy_int32 (offset); + offsetv = caml_copy_int64 (offset); flagsv = Val_flags (flags); value args[] = { *(value *) h, countv, offsetv, flagsv }; @@ -678,7 +678,7 @@ zero_wrapper (void *h, uint32_t count, uint64_t offset, uint32_t flags) LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE (); countv = caml_copy_int32 (count); - offsetv = caml_copy_int32 (offset); + offsetv = caml_copy_int64 (offset); flagsv = Val_flags (flags); value args[] = { *(value *) h, countv, offsetv, flagsv }; @@ -700,7 +700,7 @@ extents_wrapper (void *h, uint32_t count, uint64_t offset, uint32_t flags, LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE (); countv = caml_copy_int32 (count); - offsetv = caml_copy_int32 (offset); + offsetv = caml_copy_int64 (offset); flagsv = Val_flags (flags); value args[] = { *(value *) h, countv, offsetv, flagsv }; @@ -740,7 +740,7 @@ cache_wrapper (void *h, uint32_t count, uint64_t offset, uint32_t flags) LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE (); countv = caml_copy_int32 (count); - offsetv = caml_copy_int32 (offset); + offsetv = caml_copy_int64 (offset); flagsv = Val_flags (flags); value args[] = { *(value *) h, countv, offsetv, flagsv }; -- 2.32.0

The count parameter represents the length of the buffer that must be created and returned by this function. So OCaml int is the natural type for this, since the size of OCaml buffers and arrays on all platforms are represented by an int (roughly the equivalent of size_t in C). Note that signedness and/or overflow of the previous int32 type is not the concern here (unlike the previous commit). A memory buffer cannot be larger than is represented by an OCaml int. --- plugins/ocaml/NBDKit.mli | 2 +- plugins/ocaml/example.ml | 1 - plugins/ocaml/plugin.c | 2 +- tests/cc_shebang.ml | 1 - tests/test_ocaml_errorcodes_plugin.ml | 2 +- tests/test_ocaml_plugin.ml | 1 - 6 files changed, 3 insertions(+), 6 deletions(-) diff --git a/plugins/ocaml/NBDKit.mli b/plugins/ocaml/NBDKit.mli index dc83c9e9..b20c3447 100644 --- a/plugins/ocaml/NBDKit.mli +++ b/plugins/ocaml/NBDKit.mli @@ -108,7 +108,7 @@ val register_plugin : ?is_rotational: ('a -> bool) -> (* Serving data. *) - pread: ('a -> int32 -> int64 -> flags -> string) -> + pread: ('a -> int -> int64 -> flags -> string) -> ?pwrite: ('a -> string -> int64 -> flags -> unit) -> ?flush: ('a -> flags -> unit) -> ?trim: ('a -> int64 -> int64 -> flags -> unit) -> diff --git a/plugins/ocaml/example.ml b/plugins/ocaml/example.ml index c0ccb1f6..11b57747 100644 --- a/plugins/ocaml/example.ml +++ b/plugins/ocaml/example.ml @@ -75,7 +75,6 @@ let get_size h = Int64.of_int (Bytes.length !disk) let pread h count offset _ = - let count = Int32.to_int count in let buf = Bytes.create count in Bytes.blit !disk (Int64.to_int offset) buf 0 count; Bytes.unsafe_to_string buf diff --git a/plugins/ocaml/plugin.c b/plugins/ocaml/plugin.c index 74550993..e84e381b 100644 --- a/plugins/ocaml/plugin.c +++ b/plugins/ocaml/plugin.c @@ -588,7 +588,7 @@ pread_wrapper (void *h, void *buf, uint32_t count, uint64_t offset, mlsize_t len; LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE (); - countv = caml_copy_int32 (count); + countv = Val_int (count); offsetv = caml_copy_int64 (offset); flagsv = Val_flags (flags); diff --git a/tests/cc_shebang.ml b/tests/cc_shebang.ml index 644c436a..d78d7661 100755 --- a/tests/cc_shebang.ml +++ b/tests/cc_shebang.ml @@ -25,7 +25,6 @@ let open_connection _ = () let get_size () = Bytes.length !disk |> Int64.of_int let pread () count offset _ = - let count = Int32.to_int count in let buf = Bytes.create count in Bytes.blit !disk (Int64.to_int offset) buf 0 count; Bytes.unsafe_to_string buf diff --git a/tests/test_ocaml_errorcodes_plugin.ml b/tests/test_ocaml_errorcodes_plugin.ml index bb877a4d..e2c016ff 100644 --- a/tests/test_ocaml_errorcodes_plugin.ml +++ b/tests/test_ocaml_errorcodes_plugin.ml @@ -43,7 +43,7 @@ let pread () count offset _ = * error code. *) match (Int64.to_int offset) / sector_size with - | 0 -> (* good, return data *) String.make (Int32.to_int count) '\000' + | 0 -> (* good, return data *) String.make count '\000' | 1 -> NBDKit.set_error EPERM; failwith "EPERM" | 2 -> NBDKit.set_error EIO; failwith "EIO" | 3 -> NBDKit.set_error ENOMEM; failwith "ENOMEM" diff --git a/tests/test_ocaml_plugin.ml b/tests/test_ocaml_plugin.ml index 1a948bb9..9a944b46 100644 --- a/tests/test_ocaml_plugin.ml +++ b/tests/test_ocaml_plugin.ml @@ -116,7 +116,6 @@ let get_size h = let pread h count offset _ = assert (h.h_id > 0); assert (h.h_sentinel = "TESTING"); - let count = Int32.to_int count in let buf = Bytes.create count in Bytes.blit disk (Int64.to_int offset) buf 0 count; Bytes.unsafe_to_string buf -- 2.32.0