thr3ads.net - Libguestfs - [Libguestfs] [PATCH nbdkit 0/3] ocaml: Fix handling of parameter types [Jan 2022]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2022-Jan-15 12:35 UTC

[Libguestfs] [PATCH nbdkit 0/3] ocaml: Fix handling of parameter types

I've already pushed this, it's FYI only.

This is really a follow up to Laszlo Ersek's fixes to the OCaml
bindings in libnbd
(https://listman.redhat.com/archives/libguestfs/2022-January/msg00093.html)

Firstly there's a huge mistake: We were using caml_copy_int32 to
construct int64 offsets -- Ooops.  This actually works (to some
extent) on little endian platforms.

The second and third patches deal with count parameters, and therefore
possibly overlap with Eric Blake's work.

The list of extents was already being passed using int64.

Rich.

Richard W.M. Jones

2022-Jan-15 12:35 UTC

head link

[Libguestfs] [PATCH nbdkit 1/3] ocaml: Convert offset values correctly in several callbacks

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

Richard W.M. Jones

2022-Jan-15 12:35 UTC

head link

[Libguestfs] [PATCH nbdkit 2/3] ocaml: Use int64 as the OCaml type for counts

We previously mapped the C type uint32_t count to int32.  However this
will cause a negative number to be passed for counts >= 2G.  This is
theoretically not wrong, but could obviously cause problems for
plugins which are not prepared to deal with a negative count (by
converting it to int64 and adding 0x1_0000_0000).

As well as future-proofing the plugin, it's easier for us to just pass
counts as int64.

Note I did not change pread - see next commit.
---
 plugins/ocaml/NBDKit.mli | 8 ++++----
 plugins/ocaml/plugin.c   | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/plugins/ocaml/NBDKit.mli b/plugins/ocaml/NBDKit.mli
index 198c250b..dc83c9e9 100644
--- a/plugins/ocaml/NBDKit.mli
+++ b/plugins/ocaml/NBDKit.mli
@@ -111,10 +111,10 @@ val register_plugin :
   pread: ('a -> int32 -> int64 -> flags -> string) ->
   ?pwrite: ('a -> string -> int64 -> flags -> unit) ->
   ?flush: ('a -> flags -> unit) ->
-  ?trim: ('a -> int32 -> int64 -> flags -> unit) ->
-  ?zero: ('a -> int32 -> int64 -> flags -> unit) ->
-  ?extents: ('a -> int32 -> int64 -> flags -> extent list)
->
-  ?cache: ('a -> int32 -> int64 -> flags -> unit) ->
+  ?trim: ('a -> int64 -> int64 -> flags -> unit) ->
+  ?zero: ('a -> int64 -> int64 -> flags -> unit) ->
+  ?extents: ('a -> int64 -> int64 -> flags -> extent list)
->
+  ?cache: ('a -> int64 -> int64 -> flags -> unit) ->
 
   (* Miscellaneous. *)
   ?dump_plugin: (unit -> unit) ->
diff --git a/plugins/ocaml/plugin.c b/plugins/ocaml/plugin.c
index b6964046..74550993 100644
--- a/plugins/ocaml/plugin.c
+++ b/plugins/ocaml/plugin.c
@@ -656,7 +656,7 @@ trim_wrapper (void *h, uint32_t count, uint64_t offset,
uint32_t flags)
   CAMLlocal4 (rv, countv, offsetv, flagsv);
   LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE ();
 
-  countv = caml_copy_int32 (count);
+  countv = caml_copy_int64 (count);
   offsetv = caml_copy_int64 (offset);
   flagsv = Val_flags (flags);
 
@@ -677,7 +677,7 @@ zero_wrapper (void *h, uint32_t count, uint64_t offset,
uint32_t flags)
   CAMLlocal4 (rv, countv, offsetv, flagsv);
   LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE ();
 
-  countv = caml_copy_int32 (count);
+  countv = caml_copy_int64 (count);
   offsetv = caml_copy_int64 (offset);
   flagsv = Val_flags (flags);
 
@@ -699,7 +699,7 @@ extents_wrapper (void *h, uint32_t count, uint64_t offset,
uint32_t flags,
   CAMLlocal5 (rv, countv, offsetv, flagsv, v);
   LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE ();
 
-  countv = caml_copy_int32 (count);
+  countv = caml_copy_int64 (count);
   offsetv = caml_copy_int64 (offset);
   flagsv = Val_flags (flags);
 
@@ -739,7 +739,7 @@ cache_wrapper (void *h, uint32_t count, uint64_t offset,
uint32_t flags)
   CAMLlocal4 (rv, countv, offsetv, flagsv);
   LEAVE_BLOCKING_SECTION_FOR_CURRENT_SCOPE ();
 
-  countv = caml_copy_int32 (count);
+  countv = caml_copy_int64 (count);
   offsetv = caml_copy_int64 (offset);
   flagsv = Val_flags (flags);
 
-- 
2.32.0

Richard W.M. Jones

2022-Jan-15 12:35 UTC

head link

[Libguestfs] [PATCH nbdkit 3/3] ocaml: Pass count parameter of pread as OCaml int

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

Laszlo Ersek

2022-Jan-17 14:52 UTC

head link

[Libguestfs] [PATCH nbdkit 0/3] ocaml: Fix handling of parameter types

On 01/15/22 13:35, Richard W.M. Jones wrote:> I've already pushed this, it's FYI only.
looks good to me,

Reviewed-by: Laszlo Ersek <lersek at redhat.com>

And no int32 is left in "plugins/ocaml/NBDKit.mli".

Thanks
Laszlo
> This is really a follow up to Laszlo Ersek's fixes to the OCaml
> bindings in libnbd
> (https://listman.redhat.com/archives/libguestfs/2022-January/msg00093.html)
> 
> Firstly there's a huge mistake: We were using caml_copy_int32 to
> construct int64 offsets -- Ooops.  This actually works (to some
> extent) on little endian platforms.
> 
> The second and third patches deal with count parameters, and therefore
> possibly overlap with Eric Blake's work.
> 
> The list of extents was already being passed using int64.
> 
> Rich.
> 
> 
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
>

Libguestfs - Jan 2022 - [PATCH nbdkit 0/3] ocaml: Fix handling of parameter types

[Libguestfs] [PATCH nbdkit 0/3] ocaml: Fix handling of parameter types

[Libguestfs] [PATCH nbdkit 1/3] ocaml: Convert offset values correctly in several callbacks

[Libguestfs] [PATCH nbdkit 2/3] ocaml: Use int64 as the OCaml type for counts

[Libguestfs] [PATCH nbdkit 3/3] ocaml: Pass count parameter of pread as OCaml int

[Libguestfs] [PATCH nbdkit 0/3] ocaml: Fix handling of parameter types