On Thu, Nov 25, 2021 at 10:50:03AM +0200, Nir Soffer wrote:

On Wed, Nov 24, 2021 at 10:03 PM Eric Blake <eblake@redhat.com> wrote:

...

Instead of copying from a C uint32_t[] to a golang []uint32, we can exploit the fact that their underlying memory has the same representation. An unsafe cast to more memory than necessary exposes all the more that Go then needs to give a well-bounded slice, with no copying necessary.

Nice addition, I did know that we do this extra unneeded copy, and it will be nice to avoid it.

But do we document that the caller must copy the returned entries array if they need to keep it after the extent callback returns?

Oh. Ewwwww. I played with this more, and we DO have existing go unit tests that assume you can do an array assignment that survives past the callback: golang/libnbd_460_block_status_test.go: var entries []uint32 func mcf(metacontext string, offset uint64, e []uint32, error *int) int { if *error != 0 { panic("expected *error == 0") } if metacontext == "base:allocation" { entries = e } return 0 } With this patch alone applied, that test still passes, but only by luck. Our current state machine leaves h->bs_entries allocated until the next NBD_CMD_BLOCK_STATUS reply is received over the wire. But when I changed The C code state machine to free(h->bs_entries) as soon as the callback returned control to the C code, then I definitely saw the effects of the use-after-free when the rest of the Go code then saw random contents in the memory because the heap had already been repurposed. In other words, with a copy, our unit test is robust: forcing a copy causes Go to manage the array (and Go's lifetime management of the copy prevents use-after-free); but exposing an alias as in this patch puts the Go code at risk of when the C code then manipulates that array (our unit test was lucky in that it did not queue up back-to-back NBD_CMD_BLOCK_STATUS where consecutive replies from the server could interact badly, but that's not out of the realm of possibilities for a more highly-threaded client use). I don't know if there is more idiomatic Go code for forcing a copy of a transient slice for use in our glue code, but the user's Go code (including our testsuite) shouldn't have to be worried about non-idiomatic lifetimes like that. So this patch as-is is wrong.

This change may cause use after free in caller code if the code is not careful about that. I think a warning in the ExtentCallback wrapper would be useful.

This is the documentation we have now:

https://pkg.go.dev/libguestfs.org/libnbd#ExtentCallback

...

https://newbedev.com/pass-struct-and-array-of-structs-to-c-function-from-go

Maybe a better link is: https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices

Yes, that's a more canonical link.

It also explains a nicer way with Go 1.17 which we can use in a future release.

...

--- generator/GoLang.ml | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/generator/GoLang.ml b/generator/GoLang.ml index eb3aa26..7455dde 100644 --- a/generator/GoLang.ml +++ b/generator/GoLang.ml @@ -1,6 +1,6 @@ (* hey emacs, this is OCaml code: -*- tuareg -*- *) (* nbd client library in userspace: generator - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2021 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -513,13 +513,14 @@ import \"unsafe\"

/* Closures. */

-func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 { - ret := make([]uint32, int (count)) - for i := 0; i < int (count); i++ { - entry := (*C.uint32_t) (unsafe.Pointer(uintptr(unsafe.Pointer(entries)) + (unsafe.Sizeof(*entries) * uintptr(i))))

The old code is still a bunch of nasty casting. Maybe the compromise is...

...

- ret[i] = uint32 (*entry) - } - return ret +func use_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 { + /* https://stackoverflow.com/questions/48756732/what-does-1-30c-yourtype-do-exa... */ + unsafePtr := unsafe.Pointer(entries) + /* Max structured reply payload is 64M, so this array size is more than + * sufficient for the underlying slice we want to expose. + */

Why 64M?

Because that's the maximum payload we allow from the server for any response to NBD_CMD_READ or NBD_CMD_BLOCK_STATUS. Anything larger and the server is assumed to either be malicious (attempting a denial of service on the client) or we have lost communication sync with the server. Most servers won't even reach that many extents in a single reply (for example, qemu-nbd caps responses to 1M, which is at most 128k extents; see qemu/nbd/server.c NBD_MAX_BLOCK_STATUS_EXTENTS).

I guess you calculate using maximum request length (2**31 - 1) and minimum block size (512)

512-byte extents is the recommended minimum block size for an extent, but not a hard limit. Rich and I found that it was fairly easy to write an nbdkit plugin that would attempt to send more than 64M of extent information in one reply by sending alternating extents per byte (yes, sending extents that small becomes an amplification attack); and fixed it by capping the maximum extents nbdkit is willing to reply with: https://gitlab.com/nbdkit/nbdkit/-/commit/6e0dc839

so we can get up to 8388607 extents per request, with 8 bytes per extent - 67108856 bytes. If this is the reason, maybe we should include it in https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md

Both qemu and nbdkit as servers cap responses, and I don't know of any other servers that implement block status yet (upstream nbd-server does not). But yes, maybe it is worth an NBD spec tweak to explicitly mention that servers should consider capping their responses.

...

+ arrayPtr := (*[1 << 20]uint32)(unsafePtr) + return arrayPtr[:count:count]

...using the unsafePtr and arrayPtr magic for creating a nice alias variable, but then STILL performing a manual copy into Go-managed memory, and returning the copy (which is now subject to Go lifetime and garbage collection, and therefore no longer a risk of use-after-free) rather than our unsafe alias.

...

} ";

@@ -601,7 +602,7 @@ func copy_uint32_array (entries *C.uint32_t, count C.size_t) []uint32 {

This function is called copy_...

Sounds like it needs to remain a copy, rather than a use.

...

if !comma then pr ", "; comma := true; match cbarg with | CBArrayAndLen (UInt32 n, count) -> - pr "copy_uint32_array (%s, %s)" n count + pr "use_uint32_array (%s, %s)" n count

But we implement by use_... - this may confuse someone in the future, and lead to use after free.

...

| CBBytesIn (n, len) -> pr "C.GoBytes (%s, C.int (%s))" n len | CBInt n -> -- 2.33.1

Otherwise it looks good to me.

Thanks for the review pointing me in the right direction. I'm not yet sure if I will drop this entirely, or try a v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org